Обсуждение: Array initialisation notation in syscache.c

Hi,

While hacking on a new system catalogue for a nearby thread, it
occurred to me that syscache.c's table of entries could be made more
readable and less error prone.  They look like this:

        {AttributeRelationId,           /* ATTNUM */
                AttributeRelidNumIndexId,
                2,
                {
                        Anum_pg_attribute_attrelid,
                        Anum_pg_attribute_attnum,
                        0,
                        0
                },
                128
        },

Do you think this is better?

        [ATTNUM] = {
                AttributeRelationId,
                AttributeRelidNumIndexId,
                {
                        Anum_pg_attribute_attrelid,
                        Anum_pg_attribute_attnum
                },
                128
        },

We could also consider writing eg ".nbuckets  = 128", but it's not a
complicated struct that the eye gets lost in, so I didn't bother with
that in the attached.

Thomas Munro <thomas.munro@gmail.com> writes:
> Do you think this is better?

I'm not at all on board with adding runtime overhead to
save maintaining the nkeys fields.

Getting rid of the useless trailing zeroes in the key[] arrays
is clearly a win, though.

I'm kind of neutral on using "[N] = " as a substitute for
ordering the entries correctly.  While that does remove
one failure mode, it seems like it adds another (ie
failure to provide an entry at all would be masked).

            regards, tom lane

On Wed, Dec 21, 2022 at 12:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Thomas Munro <thomas.munro@gmail.com> writes:
> > Do you think this is better?
>
> I'm not at all on board with adding runtime overhead to
> save maintaining the nkeys fields.

I don't see how to do it at compile time without getting the
preprocessor involved.  What do you think about this version?

    [ATTNUM] = {
        AttributeRelationId,
        AttributeRelidNumIndexId,
        KEY(Anum_pg_attribute_attrelid,
            Anum_pg_attribute_attnum),
        128
    },

> I'm kind of neutral on using "[N] = " as a substitute for
> ordering the entries correctly.  While that does remove
> one failure mode, it seems like it adds another (ie
> failure to provide an entry at all would be masked).

It fails very early in testing if you do that.  Admittedly, the
assertion is hard to understand, but if I add a new assertion close to
the cause with a new comment to say what you did wrong, I think that
should be good enough?

On Wed, Dec 21, 2022 at 1:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>         KEY(Anum_pg_attribute_attrelid,
>             Anum_pg_attribute_attnum),

I independently rediscovered that our VA_ARGS_NARGS() macro in c.h
always returns 1 on MSVC via trial-by-CI.  Derp.  Here is the same
patch, no change from v2, but this time accompanied by Victor Spirin's
fix, which I found via one of the tab-completion-is-busted-on-Windows
discussions.  I can't supply a useful commit message, because I
haven't understood why it works, but it does indeed seem to work and
this should make cfbot green.

On 21.12.22 04:16, Thomas Munro wrote:
> On Wed, Dec 21, 2022 at 1:33 PM Thomas Munro <thomas.munro@gmail.com> wrote:
>>          KEY(Anum_pg_attribute_attrelid,
>>              Anum_pg_attribute_attnum),
> 
> I independently rediscovered that our VA_ARGS_NARGS() macro in c.h
> always returns 1 on MSVC via trial-by-CI.  Derp.  Here is the same
> patch, no change from v2, but this time accompanied by Victor Spirin's
> fix, which I found via one of the tab-completion-is-busted-on-Windows
> discussions.  I can't supply a useful commit message, because I
> haven't understood why it works, but it does indeed seem to work and
> this should make cfbot green.

This looks like a good improvement to me.

(I have also thought about having this generated from the catalog 
definition files somehow, but one step at a time ...)

Nikita Malakhov <hukutoc@gmail.com> writes:
> Wanted to ask this since I encountered a need for a cache with 5 keys -
> why is the syscache index still limited to 4 keys?

Because there are no cases requiring 5, so far.

(A unique index with as many as 5 keys seems a bit fishy btw.)

            regards, tom lane

On Thu, Dec 22, 2022 at 5:36 AM Peter Eisentraut
<peter.eisentraut@enterprisedb.com> wrote:
> This looks like a good improvement to me.

Thanks both.  Pushed.

> (I have also thought about having this generated from the catalog
> definition files somehow, but one step at a time ...)

Good plan.

From the light relief department, here is some more variadic macrology:

-       tp = SearchSysCache1(TSPARSEROID, ObjectIdGetDatum(prsId));
+       tp = SearchSysCache(TSPARSEROID, ObjectIdGetDatum(prsId));

On 31.03.23 04:16, Thomas Munro wrote:
>  From the light relief department, here is some more variadic macrology:
> 
> -       tp = SearchSysCache1(TSPARSEROID, ObjectIdGetDatum(prsId));
> +       tp = SearchSysCache(TSPARSEROID, ObjectIdGetDatum(prsId));

I'm worried that if we are removing the variants with the explicit 
numbers, it will make it difficult for extensions to maintain 
compatibility with previous PG major versions.  They would probably have 
to copy much of your syscache.h changes into their own code.  Seems messy.

On Thu, Sep 21, 2023 at 8:19 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> On 31.03.23 04:16, Thomas Munro wrote:
> >  From the light relief department, here is some more variadic macrology:
> >
> > -       tp = SearchSysCache1(TSPARSEROID, ObjectIdGetDatum(prsId));
> > +       tp = SearchSysCache(TSPARSEROID, ObjectIdGetDatum(prsId));
>
> I'm worried that if we are removing the variants with the explicit
> numbers, it will make it difficult for extensions to maintain
> compatibility with previous PG major versions.  They would probably have
> to copy much of your syscache.h changes into their own code.  Seems messy.

I suppose we could also supply a set of macros with the numbers that
map straight onto the numberless ones, with a note that they will be
deleted after N releases.  But maybe not worth the hassle for such a
tiny improvement in core code readability.  I will withdraw this
entry.  Thanks.

On 2023-Nov-14, Thomas Munro wrote:

> I suppose we could also supply a set of macros with the numbers that
> map straight onto the numberless ones, with a note that they will be
> deleted after N releases.

Maybe just keep compatibility ones with 1 and 2 arguments (the ones most
used) forever, or 15 years, and drop the rest.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
“Cuando no hay humildad las personas se degradan” (A. Christie)