MIME-Version: 1.0
References: <75ae5875-3abc-dafc-8aec-73247ed41cde@eisentraut.org>
 <CAFBsxsHMaZ9yR7p=JpzdC2ynKwkDb1PrEr=Q5M__6cmhbbt2iQ@mail.gmail.com>
 <0375b1fb-c2e7-be24-806e-b3b96bd00c31@eisentraut.org> <aaa3f9c4-9ad5-d0a1-27ae-6bcd56f5216b@eisentraut.org>
 <9aea7c02-ca6f-4cf0-0e5a-bcc0fd959b7c@eisentraut.org> <CAFBsxsEY5tYGpJ3OPEtFd_cW-=DP5WXG3tsCo+f+mAoV+WU8hw@mail.gmail.com>
 <ec503bf6-e2c0-4dff-8261-b3f6e5922c46@eisentraut.org>
In-Reply-To: <ec503bf6-e2c0-4dff-8261-b3f6e5922c46@eisentraut.org>
From: John Naylor <johncnaylorls@gmail.com>
Date: Wed, 10 Jan 2024 15:00:23 +0700
Message-ID: <CANWCAZaDbzRvGTOejr32gxNgWAD_Xs__YW2TfwVQjpRGGr-TGA@mail.gmail.com>
Subject: Re: generate syscache info automatically
To: Peter Eisentraut <peter@eisentraut.org>
Cc: pgsql-hackers <pgsql-hackers@postgresql.org>, 
	John Naylor <john.naylor@enterprisedb.com>
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable
Archived-At: <https://www.postgresql.org/message-id/CANWCAZaDbzRvGTOejr32gxNgWAD_Xs__YW2TfwVQjpRGGr-TGA%40mail.gmail.com>
Precedence: bulk

On Thu, Nov 2, 2023 at 4:13=E2=80=AFAM Peter Eisentraut <peter@eisentraut.o=
rg> wrote:
>
> Here is a rebased patch set, along with a summary of the questions I
> have about these patches:

This is an excellent summary of the issues, thanks.

> v4-0001-Generate-syscache-info-from-catalog-files.patch
>
> What's a good syntax to declare a syscache?  Currently, I have, for examp=
le
>
> -DECLARE_UNIQUE_INDEX_PKEY(pg_type_oid_index, 2703, TypeOidIndexId,
> pg_type, btree(oid oid_ops));
> +DECLARE_UNIQUE_INDEX_PKEY(pg_type_oid_index, 2703, TypeOidIndexId,
> pg_type, btree(oid oid_ops), TYPEOID, 64);
>
> I suppose a sensible alternative could be to leave the
> DECLARE_..._INDEX... alone and make a separate statement, like
>
> MAKE_SYSCACHE(pg_type_oid_index, TYPEOID, 64);
>
> That's at least visually easier, because some of those
> DECLARE_... lines are getting pretty long.

Probably a good idea, and below I mention a third possible macro.

> I would like to keep those MAKE_SYSCACHE lines in the catalog files.
> That just makes it easier to reference everything together.

That seems fine. If we ever want to do something more invasive with
the syscaches, some of that can be copied/moved out into a separate
script, but what's there in 0001 is pretty small.

> v4-0002-Generate-ObjectProperty-from-catalog-files.patch
>
> Several questions here:
>
> * What's a good way to declare the mapping between catalog and object
>    type?

Perhaps this idea:

> I suppose a sensible alternative could be to leave the
> DECLARE_..._INDEX... alone and make a separate statement, like
>
> MAKE_SYSCACHE(pg_type_oid_index, TYPEOID, 64);

...could be used, so something like

DECLARE_OBJECT_INFO(OBJECT_ACCESS_METHOD);

> * How to select which catalogs have ObjectProperty entries generated?

Would the above work for that as well?

> * Where to declare class descriptions?  Or just keep the current hack in
>    the patch.

I don't have an opinion on this.

> * How to declare the purpose of a catalog column, like "this is the ACL
>    column for this catalog".  This is currently done by name, but maybe
>    it should be more explicit.

Perhaps we can have additional column macros, like BKI_OBJ_ACL or
something, but that doesn't seem like it would result in less code.

> * Question about how to pick the correct value for is_nsp_name_unique?

How is that known now -- I don't mean in the patch, but in general?

Also, I get most of the hard-wired exceptions, but

+ # XXX?
+ $name_syscache =3D "SUBSCRIPTIONNAME" if $catname eq 'pg_subscription';

What is not right otherwise?

+ if ($index->{index_decl} eq 'btree(oid oid_ops)')
+ {
+ $oid_index =3D $index->{index_oid_macro};
+ $oid_syscache =3D $index->{syscache_name};
+ }
+ if ($index->{index_decl} =3D~ /\(\w+name name_ops(, \w+namespace oid_ops)=
?\)/)
+ {
+ $name_syscache =3D $index->{syscache_name};
+ $is_nsp_name_unique =3D 1 if $index->{is_unique};
+ }

The variables name_syscache and syscache_name are unfortunately close
to eachother.