Re: Add table AM 'tid_visible'

We directly call the heap function VM_ALL_VISIBLE in the
IndexOnlyNext function. This is not in line with the design idea of
table am. If the new storage type needs to implement index only
scan, he must hack the IndexOnlyNext function.

So this patch add a new table am 'tid_visible' to test visibility
of tid. So that index only scan can completely use table AM.

Hi,

On 2020-11-02 09:16:26 +0000, Jinbao Chen wrote:
> We directly call the heap function VM_ALL_VISIBLE in the
> IndexOnlyNext function. This is not in line with the design idea of
> table am. If the new storage type needs to implement index only
> scan, he must hack the IndexOnlyNext function.

Yea, it's something we should improve. Have you checked if this has
performance impact for heap? Should we also consider planning costs?

> So this patch add a new table am 'tid_visible' to test visibility
> of tid. So that index only scan can completely use table AM.

As far as I can tell you have not acually attached the patch.

Greetings,

Andres Freund

Hi Andres,

> Yea, it's something we should improve. Have you checked if this has

> performance impact for heap? Should we also consider planning costs?

Since the visibility map is very small, all pages of the visibility map will

usually reside in memory. The IO cost of accessing the visibility map can

be ignored. We should add the CPU cost of accessing visibility map. The

CPU cost of accessing visibility map is usually smaller than cpu_tuple_cost.

But Postgres does not have a Macro to describe such a small cost. Should

We add one?

> As far as I can tell you have not acually attached the patch.

Ah, forgot to upload the patch. Attach it below.

On Tue, Nov 3, 2020 at 5:23 PM Jinbao Chen <cjinbao(at)vmware(dot)com> wrote:
>
> Hi Andres,
>
>
>
> > Yea, it's something we should improve. Have you checked if this has
>
> > performance impact for heap? Should we also consider planning costs?
>
> Since the visibility map is very small, all pages of the visibility map will
>
> usually reside in memory. The IO cost of accessing the visibility map can
>
> be ignored. We should add the CPU cost of accessing visibility map. The
>
> CPU cost of accessing visibility map is usually smaller than cpu_tuple_cost.
>
> But Postgres does not have a Macro to describe such a small cost. Should
>
> We add one?
>
>
>
> > As far as I can tell you have not acually attached the patch.
>
> Ah, forgot to upload the patch. Attach it below.

You sent in your patch, tid_visible-1.patch to pgsql-hackers on Nov 3,
but you did not post it to the next CommitFest[1]. If this was
intentional, then you need to take no action. However, if you want
your patch to be reviewed as part of the upcoming CommitFest, then you
need to add it yourself before 2021-01-01 AoE[2]. Thanks for your
contributions.

Regards,

[1] https://commitfest.postgresql.org/31/
[2] https://en.wikipedia.org/wiki/Anywhere_on_Earth

--
Masahiko Sawada
EnterpriseDB: https://www.enterprisedb.com/