Re: generate syscache info automatically

On Thu, Nov 2, 2023 at 4:13 AM Peter Eisentraut <peter(at)eisentraut(dot)org> 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 example
>
> -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 = "SUBSCRIPTIONNAME" if $catname eq 'pg_subscription';

What is not right otherwise?

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

The variables name_syscache and syscache_name are unfortunately close
to eachother.