Horton

Thread

Commits

  1. Remove ObjectClass type

  2. Simplify pg_enc2gettext_tbl[] with C99-designated initializer syntax

  3. Use C99-designated initializer syntax for arrays related to encodings

  4. Remove last NULL element in config_group_names[]

  5. Use C99-designated initializer syntax for more arrays

  6. Remove unnecessary array object_classes[] in dependency.c

  7. Add trailing commas to enum definitions

  1. Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-21T15:03:31Z

    Usage of designated initializers came up in:
https://www.postgresql.org/message-id/flat/ZdWXhAt9Tz4d-lut%40paquier.xyz#9dc17e604e58569ad35643672bf74acc

This converts all arrays that I could find that could clearly benefit
from this without any other code changes being necessary.

There were a few arrays that I didn't convert that seemed like they
could be useful to convert, but where the variables started counting
at 1. So by converting them elements the array would grow and elements
would be shifted by one. Changing those might be nice, but would
require some more code changes so I didn't want to combine it with
these simpler refactors. The arrays I'm talking about were
specifically tsearch_op_priority, BT_implies_table, BT_refutes_table,
and BT_implic_table.

  2. Re: Improve readability by using designated initializers when possible

    Jeff Davis <pgsql@j-davis.com> — 2024-02-22T22:46:51Z

    On Wed, 2024-02-21 at 16:03 +0100, Jelte Fennema-Nio wrote:
> Usage of designated initializers came up in:
> https://www.postgresql.org/message-id/flat/ZdWXhAt9Tz4d-lut%40paquier.xyz#9dc17e604e58569ad35643672bf74acc
> 
> This converts all arrays that I could find that could clearly benefit
> from this without any other code changes being necessary.

Looking at the object_classes array and the ObjectClass enum, I don't
quite understand the point. It seems like a way to write OCLASS_OPCLASS
instead of OperatorClassRelationId, and similar?

Am I missing something?

Regards,
	Jeff Davis

  3. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-23T00:35:36Z

    On Thu, Feb 22, 2024, 23:46 Jeff Davis <pgsql@j-davis.com> wrote:

>
> Am I missing something?


The main benefits it has are:
1. The order of the array doesn't have to exactly match the order of the
enum for the arrays to contain the correct mapping.
2. Typos in the enum variant names are caught by the compiler because
actual symbols are used, not comments.
3. The left-to-right order reads more natural imho for such key-value
pairs, e.g. OCLASS_PROC maps to ProcedureRelationId.

  4. Re: Improve readability by using designated initializers when possible

    Jeff Davis <pgsql@j-davis.com> — 2024-02-23T01:57:14Z

    On Fri, 2024-02-23 at 01:35 +0100, Jelte Fennema-Nio wrote:
> On Thu, Feb 22, 2024, 23:46 Jeff Davis <pgsql@j-davis.com> wrote:
> > 
> > Am I missing something?
> 
> The main benefits it has are:

Sorry, I was unclear. I was asking a question about the reason the
ObjectClass and the object_classes[] array exist in the current code,
it wasn't a direct question about your patch.

ObjectClass is only used in a couple places, and I don't immediately
see why those places can't just use the OID of the class (like 
OperatorClassRelationId).

Regards,
	Jeff Davis

  5. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-23T09:59:53Z

    On Fri, 23 Feb 2024 at 02:57, Jeff Davis <pgsql@j-davis.com> wrote:
> Sorry, I was unclear. I was asking a question about the reason the
> ObjectClass and the object_classes[] array exist in the current code,
> it wasn't a direct question about your patch.

I did a bit of git spelunking and the reason seems to be that back in
2002 when this was introduced not all relation ids were compile time
constants and thus an array was initialized once at bootup. I totally
agree with you that these days there's no reason for the array. So I
now added a second patch that removes this array, instead of updating
it to use the designated initializer syntax.

  6. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-02-26T08:41:17Z

    Hi. minor issues.

@@ -2063,12 +2009,12 @@ find_expr_references_walker(Node *node,
  CoerceViaIO *iocoerce = (CoerceViaIO *) node;

  /* since there is no exposed function, need to depend on type */
- add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
+ add_object_address(TypeRelationId iocoerce->resulttype, 0,
    context->addrs);

@@ -2090,21 +2036,21 @@ find_expr_references_walker(Node *node,
  ConvertRowtypeExpr *cvt = (ConvertRowtypeExpr *) node;

  /* since there is no function dependency, need to depend on type */
- add_object_address(OCLASS_TYPE, cvt->resulttype, 0,
+ add_object_address(TypeRelationId cvt->resulttype, 0,
    context->addrs);

obvious typo errors.

diff --git a/src/common/relpath.c b/src/common/relpath.c
index b16fe19dea6..d9214f915c9 100644
--- a/src/common/relpath.c
+++ b/src/common/relpath.c
@@ -31,10 +31,10 @@
  * pg_relation_size().
  */
 const char *const forkNames[] = {
- "main", /* MAIN_FORKNUM */
- "fsm", /* FSM_FORKNUM */
- "vm", /* VISIBILITYMAP_FORKNUM */
- "init" /* INIT_FORKNUM */
+ [MAIN_FORKNUM] = "main",
+ [FSM_FORKNUM] = "fsm",
+ [VISIBILITYMAP_FORKNUM] = "vm",
+ [INIT_FORKNUM] = "init",
 };

`+ [INIT_FORKNUM] = "init", ` no need for an extra  comma?

+ [PG_SJIS] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
pg_sjis_verifychar, pg_sjis_verifystr, 2},
+ [PG_BIG5] = {0, 0, pg_big5_mblen, pg_big5_dsplen,
pg_big5_verifychar, pg_big5_verifystr, 2},
+ [PG_GBK] = {0, 0, pg_gbk_mblen, pg_gbk_dsplen, pg_gbk_verifychar,
pg_gbk_verifystr, 2},
+ [PG_UHC] = {0, 0, pg_uhc_mblen, pg_uhc_dsplen, pg_uhc_verifychar,
pg_uhc_verifystr, 2},
+ [PG_GB18030] = {0, 0, pg_gb18030_mblen, pg_gb18030_dsplen,
pg_gb18030_verifychar, pg_gb18030_verifystr, 4},
+ [PG_JOHAB] = {0, 0, pg_johab_mblen, pg_johab_dsplen,
pg_johab_verifychar, pg_johab_verifystr, 3},
+ [PG_SHIFT_JIS_2004] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
pg_sjis_verifychar, pg_sjis_verifystr, 2},
 };
similarly, last entry, no need an extra comma?
also other places last array entry no need extra comma.

  7. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-02-26T09:00:13Z

    On Mon, 26 Feb 2024 at 16:41, jian he <jian.universality@gmail.com> wrote:
> Hi. minor issues.
>
> @@ -2063,12 +2009,12 @@ find_expr_references_walker(Node *node,
>   CoerceViaIO *iocoerce = (CoerceViaIO *) node;
>
>   /* since there is no exposed function, need to depend on type */
> - add_object_address(OCLASS_TYPE, iocoerce->resulttype, 0,
> + add_object_address(TypeRelationId iocoerce->resulttype, 0,
>     context->addrs);
>
> @@ -2090,21 +2036,21 @@ find_expr_references_walker(Node *node,
>   ConvertRowtypeExpr *cvt = (ConvertRowtypeExpr *) node;
>
>   /* since there is no function dependency, need to depend on type */
> - add_object_address(OCLASS_TYPE, cvt->resulttype, 0,
> + add_object_address(TypeRelationId cvt->resulttype, 0,
>     context->addrs);
>
> obvious typo errors.
>
> diff --git a/src/common/relpath.c b/src/common/relpath.c
> index b16fe19dea6..d9214f915c9 100644
> --- a/src/common/relpath.c
> +++ b/src/common/relpath.c
> @@ -31,10 +31,10 @@
>   * pg_relation_size().
>   */
>  const char *const forkNames[] = {
> - "main", /* MAIN_FORKNUM */
> - "fsm", /* FSM_FORKNUM */
> - "vm", /* VISIBILITYMAP_FORKNUM */
> - "init" /* INIT_FORKNUM */
> + [MAIN_FORKNUM] = "main",
> + [FSM_FORKNUM] = "fsm",
> + [VISIBILITYMAP_FORKNUM] = "vm",
> + [INIT_FORKNUM] = "init",
>  };
>
> `+ [INIT_FORKNUM] = "init", ` no need for an extra  comma?
>
> + [PG_SJIS] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
> pg_sjis_verifychar, pg_sjis_verifystr, 2},
> + [PG_BIG5] = {0, 0, pg_big5_mblen, pg_big5_dsplen,
> pg_big5_verifychar, pg_big5_verifystr, 2},
> + [PG_GBK] = {0, 0, pg_gbk_mblen, pg_gbk_dsplen, pg_gbk_verifychar,
> pg_gbk_verifystr, 2},
> + [PG_UHC] = {0, 0, pg_uhc_mblen, pg_uhc_dsplen, pg_uhc_verifychar,
> pg_uhc_verifystr, 2},
> + [PG_GB18030] = {0, 0, pg_gb18030_mblen, pg_gb18030_dsplen,
> pg_gb18030_verifychar, pg_gb18030_verifystr, 4},
> + [PG_JOHAB] = {0, 0, pg_johab_mblen, pg_johab_dsplen,
> pg_johab_verifychar, pg_johab_verifystr, 3},
> + [PG_SHIFT_JIS_2004] = {0, 0, pg_sjis_mblen, pg_sjis_dsplen,
> pg_sjis_verifychar, pg_sjis_verifystr, 2},
>  };
> similarly, last entry, no need an extra comma?
> also other places last array entry no need extra comma.

For last entry comma, see [1].

[1] https://www.postgresql.org/message-id/386f8c45-c8ac-4681-8add-e3b0852c1620%40eisentraut.org

  8. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-02-27T05:20:36Z

    On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote:
> On Mon, 26 Feb 2024 at 16:41, jian he <jian.universality@gmail.com> wrote:
>> similarly, last entry, no need an extra comma?
>> also other places last array entry no need extra comma.
> 
> For last entry comma, see [1].
> 
> [1] https://www.postgresql.org/message-id/386f8c45-c8ac-4681-8add-e3b0852c1620%40eisentraut.org

And also see commit 611806cd726f.  This makes the diffs more elegant
to the eye when adding new elements at the end of these arrays.
--
Michael

  9. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-02-27T06:25:35Z

    On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote:
> On Mon, 26 Feb 2024 at 16:41, jian he <jian.universality@gmail.com> wrote:
>> obvious typo errors.

These would cause compilation failures.  Saying that, this is a very
nice cleanup, so I've fixed these and applied the patch after checking
that the one-one replacements were correct.

About 0002, I can't help but notice pg_enc2icu_tbl and
pg_enc2gettext_tb.  There are exceptions like MULE_INTERNAL, but is it
possible to do better?
--
Michael

  10. Re: Improve readability by using designated initializers when possible

    Alvaro Herrera <alvherre@alvh.no-ip.org> — 2024-02-27T07:57:31Z

    On 2024-Feb-27, Michael Paquier wrote:

> These would cause compilation failures.  Saying that, this is a very
> nice cleanup, so I've fixed these and applied the patch after checking
> that the one-one replacements were correct.

Oh, I thought we were going to get rid of ObjectClass altogether -- I
mean, have getObjectClass() return ObjectAddress->classId, and then
define the OCLASS values for each catalog OID [... tries to ...]  But
this(*) doesn't work for two reasons:

1. some switches processing the OCLASS enum don't have "default:" cases.
This is so that the compiler tells us when we fail to add support for
some new object class (and it's been helpful).  If we were to 

2. all users of getObjectClass would have to include the catalog header
specific to every catalog it wants to handle; so tablecmds.c and
dependency.c would have to include almost all catalog includes, for
example.

What this says to me is that ObjectClass is/was a somewhat useful
abstraction layer on top of catalog definitions.  I'm now not 100% that
poking this hole in the abstraction (by expanding use of catalog OIDs at
the expense of ObjectClass) was such a great idea.  Maybe we want to
make ObjectClass *more* useful/encompassing rather than the opposite.


(*) I mean

Oid
getObjectClass(const ObjectAddress *object)
{
    /* only pg_class entries can have nonzero objectSubId */
    if (object->classId != RelationRelationId &&
        object->objectSubId != 0)
        elog(ERROR, "invalid non-zero objectSubId for object class %u",
             object->classId);

    return object->classId;
}

plus

#define OCLASS_CLASS RelationRelationId
#define OCLASS_PROC ProcedureRelationId
#define OCLASS_TYPE TypeRelationId

etc.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Bob [Floyd] used to say that he was planning to get a Ph.D. by the "green
stamp method," namely by saving envelopes addressed to him as 'Dr. Floyd'.
After collecting 500 such letters, he mused, a university somewhere in
Arizona would probably grant him a degree.              (Don Knuth)

  11. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-27T11:07:58Z

    On Tue, 27 Feb 2024 at 07:25, Michael Paquier <michael@paquier.xyz> wrote:
>
> On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote:
> > On Mon, 26 Feb 2024 at 16:41, jian he <jian.universality@gmail.com> wrote:
> >> obvious typo errors.
>
> These would cause compilation failures.  Saying that, this is a very
> nice cleanup, so I've fixed these and applied the patch after checking
> that the one-one replacements were correct.

Sorry about those search/replace mistakes. Not sure how that happened.
Thanks for committing :)

  12. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-27T11:52:22Z

    On Tue, 27 Feb 2024 at 07:25, Michael Paquier <michael@paquier.xyz> wrote:
> About 0002, I can't help but notice pg_enc2icu_tbl and
> pg_enc2gettext_tb.  There are exceptions like MULE_INTERNAL, but is it
> possible to do better?

Attached is an updated patchset to also convert pg_enc2icu_tbl and
pg_enc2gettext_tbl. I converted pg_enc2gettext_tbl in a separate
commit, because it actually requires some codechanges too.

  13. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-27T11:55:34Z

    On Tue, 27 Feb 2024 at 12:52, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> Attached is an updated patchset to also convert pg_enc2icu_tbl and
> pg_enc2gettext_tbl. I converted pg_enc2gettext_tbl in a separate
> commit, because it actually requires some codechanges too.

Another small update to also make all arrays changed by this patch
have a trailing comma (to avoid future diff noise).

  14. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-27T12:29:57Z

    On Tue, 27 Feb 2024 at 08:57, Alvaro Herrera <alvherre@alvh.no-ip.org> wrote:
> What this says to me is that ObjectClass is/was a somewhat useful
> abstraction layer on top of catalog definitions.  I'm now not 100% that
> poking this hole in the abstraction (by expanding use of catalog OIDs at
> the expense of ObjectClass) was such a great idea.  Maybe we want to
> make ObjectClass *more* useful/encompassing rather than the opposite.

I agree that ObjectClass has some benefits over using the table OIDs,
but both the benefits you mention don't apply to add_object_address.
So I don't think using ObjectClass for was worth the extra effort to
maintain the
object_classes array, just for add_object_address.

One improvement that I think could be worth considering is to link
ObjectClass and the table OIDs more explicitly, by actually making
their values the same:
enum ObjectClass {
    OCLASS_PGCLASS = RelationRelationId,
    OCLASS_PGPROC = ProcedureRelationId,
    ...
}

But that would effectively mean that anyone including dependency.h
would also be including all catalog headers. I'm not sure if that's
considered problematic or not. If that is problematic then it would
also be possible to reverse the relationship and have each catalog
header include dependency.h (or some other header that we move
ObjectClass to), and go about it in the following way:

/* dependency.h */
enum ObjectClass {
    OCLASS_PGCLASS = 1259,
    OCLASS_PGPROC = 1255,
    ...
}

/* pg_class.h */
CATALOG(pg_class,OCLASS_PGCLASS,RelationRelationId) BKI_BOOTSTRAP
BKI_ROWTYPE_OID(83,RelationRelation_Rowtype_Id) BKI_SCHEMA_MACRO

  15. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-02-27T15:04:34Z

    On Tue, 27 Feb 2024 at 19:55, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Tue, 27 Feb 2024 at 12:52, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> Attached is an updated patchset to also convert pg_enc2icu_tbl and
>> pg_enc2gettext_tbl. I converted pg_enc2gettext_tbl in a separate
>> commit, because it actually requires some codechanges too.
>
> Another small update to also make all arrays changed by this patch
> have a trailing comma (to avoid future diff noise).

I see the config_group_names[] needs null-terminated because of help_config,
however, I didn't find the reference in help_config.c.  Is this comment
outdated?  Here is a patch to remove the null-terminated.

diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 59904fd007..df849f73fc 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -715,11 +715,9 @@ const char *const config_group_names[] =
 	[PRESET_OPTIONS] = gettext_noop("Preset Options"),
 	[CUSTOM_OPTIONS] = gettext_noop("Customized Options"),
 	[DEVELOPER_OPTIONS] = gettext_noop("Developer Options"),
-	/* help_config wants this array to be null-terminated */
-	NULL
 };

-StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 2),
+StaticAssertDecl(lengthof(config_group_names) == (DEVELOPER_OPTIONS + 1),
 				 "array length mismatch");

 /*

  16. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-27T16:06:47Z

    On Tue, 27 Feb 2024 at 16:04, Japin Li <japinli@hotmail.com> wrote:
> I see the config_group_names[] needs null-terminated because of help_config,
> however, I didn't find the reference in help_config.c.  Is this comment
> outdated?

Yeah, you're correct. That comment has been outdated for more than 20
years. The commit that made it unnecessary to null-terminate the array
was 9d77708d83ee.

Attached is v5 of the patchset that also includes this change (with
you listed as author).

  17. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-02-28T01:41:42Z

    On Wed, 28 Feb 2024 at 00:06, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Tue, 27 Feb 2024 at 16:04, Japin Li <japinli@hotmail.com> wrote:
>> I see the config_group_names[] needs null-terminated because of help_config,
>> however, I didn't find the reference in help_config.c.  Is this comment
>> outdated?
>
> Yeah, you're correct. That comment has been outdated for more than 20
> years. The commit that made it unnecessary to null-terminate the array
> was 9d77708d83ee.
>
> Attached is v5 of the patchset that also includes this change (with
> you listed as author).

Thanks for updating the patch!

It looks good to me except there is an outdated comment.

diff --git a/src/common/encnames.c b/src/common/encnames.c
index bd012fe3a0..dba6bd2c9e 100644
--- a/src/common/encnames.c
+++ b/src/common/encnames.c
@@ -297,7 +297,6 @@ static const pg_encname pg_encname_tbl[] =

 /* ----------
  * These are "official" encoding names.
- * XXX must be sorted by the same order as enum pg_enc (in mb/pg_wchar.h)
  * ----------
  */
 #ifndef WIN32

  18. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-02-28T03:59:24Z

    On Wed, Feb 28, 2024 at 09:41:42AM +0800, Japin Li wrote:
> On Wed, 28 Feb 2024 at 00:06, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> Attached is v5 of the patchset that also includes this change (with
>> you listed as author).
> 
> Thanks for updating the patch!

Cool.  I have applied 0004 and most of 0002.  Attached is what
remains, where I am wondering if it would be cleaner to do these bits
together (did not look at the whole, yet).

> It looks good to me except there is an outdated comment.

Yep, I've updated that in the attached for now.
--
Michael

  19. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-28T04:37:22Z

    On Wed, 28 Feb 2024 at 04:59, Michael Paquier <michael@paquier.xyz> wrote:
> Cool.  I have applied 0004 and most of 0002.  Attached is what
> remains, where I am wondering if it would be cleaner to do these bits
> together (did not look at the whole, yet).

Feel free to squash them if you prefer that. I think now that patch
0002 only includes encoding changes, I feel 50-50 about grouping them
together. I mainly kept them separate, because 0002 were simple search
+ replaces and 0003 required code changes. That's still the case, but
now I can also see the argument for grouping them together since that
would clean up all the encoding arrays in one single commit (without a
ton of other arrays also being modified).

  20. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-02-29T00:56:55Z

    On Wed, Feb 28, 2024 at 05:37:22AM +0100, Jelte Fennema-Nio wrote:
> On Wed, 28 Feb 2024 at 04:59, Michael Paquier <michael@paquier.xyz> wrote:
>> Cool.  I have applied 0004 and most of 0002.  Attached is what
>> remains, where I am wondering if it would be cleaner to do these bits
>> together (did not look at the whole, yet).
> 
> Feel free to squash them if you prefer that. I think now that patch
> 0002 only includes encoding changes, I feel 50-50 about grouping them
> together. I mainly kept them separate, because 0002 were simple search
> + replaces and 0003 required code changes. That's still the case, but
> now I can also see the argument for grouping them together since that
> would clean up all the encoding arrays in one single commit (without a
> ton of other arrays also being modified).

I have doubts about the changes in raw_pg_bind_textdomain_codeset(),
as the encoding could come from the value in the pg_database tuples
themselves.  The current coding is slightly safer from the perspective
of bogus input values as we would loop over pg_enc2gettext_tbl looking
for a match.  0003 changes that so as we could point to incorrect
memory areas rather than fail safely for the NULL check.

That's not something that shows up as a problem for all the other
structures that have been changed afd8ef39094b or ef5e2e90859a.
That's not an issue for pg_enc2name_tbl, pg_enc2icu_tbl and
pg_wchar_table either thanks to PG_VALID(_{BE,FE})_ENCODING()
that offer protection with the index values used for the table
lookups.

- * WARNING: the order of this enum must be same as order of entries
- *            in the pg_enc2name_tbl[] array (in src/common/encnames.c), and
- *            in the pg_wchar_table[] array (in src/common/wchar.c)!
- *
- *            If you add some encoding don't forget to check
+ * WARNING: If you add some encoding don't forget to check
  *            PG_ENCODING_BE_LAST macro.

Mentioning the updates to pg_enc2name_tbl[] and pg_wchar_table[] is
still important, IMO, because new encoding values added to the central
enum would cause the lookups of the tables to fail while passing the
PG_VALID checks, so updating them is mandatory and could be missed.
I've tweaked the comment to mention both of them; the order does not
matter anymore.  Applied 0002 with these adjustments.
--
Michael

  21. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-02-29T03:01:47Z

    On Thu, 29 Feb 2024 at 01:57, Michael Paquier <michael@paquier.xyz> wrote:
> I have doubts about the changes in raw_pg_bind_textdomain_codeset(),
> as the encoding could come from the value in the pg_database tuples
> themselves.  The current coding is slightly safer from the perspective
> of bogus input values as we would loop over pg_enc2gettext_tbl looking
> for a match.  0003 changes that so as we could point to incorrect
> memory areas rather than fail safely for the NULL check.

That's fair. Attached is a patch that adds a PG_VALID_ENCODING check
to raw_pg_bind_textdomain_codeset to solve this regression.

  22. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-02-29T11:41:38Z

    On 27.02.24 08:57, Alvaro Herrera wrote:
> On 2024-Feb-27, Michael Paquier wrote:
> 
>> These would cause compilation failures.  Saying that, this is a very
>> nice cleanup, so I've fixed these and applied the patch after checking
>> that the one-one replacements were correct.
> 
> Oh, I thought we were going to get rid of ObjectClass altogether -- I
> mean, have getObjectClass() return ObjectAddress->classId, and then
> define the OCLASS values for each catalog OID [... tries to ...]  But
> this(*) doesn't work for two reasons:

I have long wondered what the point of ObjectClass is.  I find the extra 
layer of redirection, which is used only in small parts of the code, and 
the similarity to ObjectType confusing.  I happened to have a draft 
patch for its removal lying around, so I'll show it here, rebased over 
what has already been done in this thread.

> 1. some switches processing the OCLASS enum don't have "default:" cases.
> This is so that the compiler tells us when we fail to add support for
> some new object class (and it's been helpful).  If we were to

I think you can also handle that with some assertions and proper test 
coverage.  It's not even clear how strong this benefit is.  For example, 
in AlterObjectNamespace_oid(), you could still put a new OCLASS into the 
"ignore object types that don't have schema-qualified names" case, and 
it might or might not be wrong.  Also, there are already various OCLASS 
switches that do have a default case, so it's not even clear what the 
preferred coding style should be.

> 2. all users of getObjectClass would have to include the catalog header
> specific to every catalog it wants to handle; so tablecmds.c and
> dependency.c would have to include almost all catalog includes, for
> example.

This doesn't seem to be a problem in practice; see patch.

  23. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-01T04:08:15Z

    On Thu, Feb 29, 2024 at 12:41:38PM +0100, Peter Eisentraut wrote:
> On 27.02.24 08:57, Alvaro Herrera wrote:
>> On 2024-Feb-27, Michael Paquier wrote:
>>> These would cause compilation failures.  Saying that, this is a very
>>> nice cleanup, so I've fixed these and applied the patch after checking
>>> that the one-one replacements were correct.
>> 
>> Oh, I thought we were going to get rid of ObjectClass altogether -- I
>> mean, have getObjectClass() return ObjectAddress->classId, and then
>> define the OCLASS values for each catalog OID [... tries to ...]  But
>> this(*) doesn't work for two reasons:
> 
> I have long wondered what the point of ObjectClass is.  I find the extra
> layer of redirection, which is used only in small parts of the code, and the
> similarity to ObjectType confusing.  I happened to have a draft patch for
> its removal lying around, so I'll show it here, rebased over what has
> already been done in this thread.

The elimination of getObjectClass() seems like a good end goal IMO, so
the direction of the patch is interesting.  Would object_type_map and
ObjectProperty follow the same idea of relying on the catalogs OID
instead of the OBJECT_*?

Note that there are still two dependencies to getObjectClass() in
event_trigger.c and dependency.c.
--
Michael

  24. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-01T04:12:35Z

    On Thu, Feb 29, 2024 at 04:01:47AM +0100, Jelte Fennema-Nio wrote:
> On Thu, 29 Feb 2024 at 01:57, Michael Paquier <michael@paquier.xyz> wrote:
>> I have doubts about the changes in raw_pg_bind_textdomain_codeset(),
>> as the encoding could come from the value in the pg_database tuples
>> themselves.  The current coding is slightly safer from the perspective
>> of bogus input values as we would loop over pg_enc2gettext_tbl looking
>> for a match.  0003 changes that so as we could point to incorrect
>> memory areas rather than fail safely for the NULL check.
> 
> That's fair. Attached is a patch that adds a PG_VALID_ENCODING check
> to raw_pg_bind_textdomain_codeset to solve this regression.

-   for (i = 0; pg_enc2gettext_tbl[i].name != NULL; i++)
+   if (!PG_VALID_ENCODING(encoding) || pg_enc2gettext_tbl[encoding] == NULL)     { 

Shouldn't PG_MULE_INTERNAL point to NULL in pg_enc2gettext_tbl[]?
That just seems safer to me, and more consistent because its values
satisfies PG_VALID_ENCODING().
--
Michael

  25. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-01T04:34:05Z

    On Fri, 1 Mar 2024 at 05:12, Michael Paquier <michael@paquier.xyz> wrote:
> Shouldn't PG_MULE_INTERNAL point to NULL in pg_enc2gettext_tbl[]?
> That just seems safer to me, and more consistent because its values
> satisfies PG_VALID_ENCODING().

Safety wise it doesn't matter, because gaps in a designated
initializer array will be initialized with 0/NULL. But I agree it's
more consistent, so see attached.

  26. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-01T05:08:40Z

    On Fri, Mar 01, 2024 at 05:34:05AM +0100, Jelte Fennema-Nio wrote:

> diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
> index fd91aefbcb7..32e25a1a6ea 100644
> --- a/src/include/mb/pg_wchar.h
> +++ b/src/include/mb/pg_wchar.h
> @@ -225,7 +225,8 @@ typedef unsigned int pg_wchar;
>   * PostgreSQL encoding identifiers
>   *
>   * WARNING: If you add some encoding don't forget to update
> - *			the pg_enc2name_tbl[] array (in src/common/encnames.c) and
> + *			the pg_enc2name_tbl[] array (in src/common/encnames.c),
> + *			the pg_enc2gettext_tbl[] array (in src/common/encnames.c) and
>   *			the pg_wchar_table[] array (in src/common/wchar.c) and to check
>   *			PG_ENCODING_BE_LAST macro.

Mostly OK to me.  Just note that this comment is incorrect because
pg_enc2gettext_tbl[] includes elements in the range
[PG_SJIS,_PG_LAST_ENCODING_[  ;)
--
Michael

  27. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-01T05:30:10Z

    On Fri, 1 Mar 2024 at 06:08, Michael Paquier <michael@paquier.xyz> wrote:
> Mostly OK to me.  Just note that this comment is incorrect because
> pg_enc2gettext_tbl[] includes elements in the range
> [PG_SJIS,_PG_LAST_ENCODING_[  ;)

fixed

  28. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-03-01T07:03:37Z

    On Fri, Mar 1, 2024 at 12:08 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Thu, Feb 29, 2024 at 12:41:38PM +0100, Peter Eisentraut wrote:
> > On 27.02.24 08:57, Alvaro Herrera wrote:
> >> On 2024-Feb-27, Michael Paquier wrote:
> >>> These would cause compilation failures.  Saying that, this is a very
> >>> nice cleanup, so I've fixed these and applied the patch after checking
> >>> that the one-one replacements were correct.
> >>
> >> Oh, I thought we were going to get rid of ObjectClass altogether -- I
> >> mean, have getObjectClass() return ObjectAddress->classId, and then
> >> define the OCLASS values for each catalog OID [... tries to ...]  But
> >> this(*) doesn't work for two reasons:
> >
> > I have long wondered what the point of ObjectClass is.  I find the extra
> > layer of redirection, which is used only in small parts of the code, and the
> > similarity to ObjectType confusing.  I happened to have a draft patch for
> > its removal lying around, so I'll show it here, rebased over what has
> > already been done in this thread.
>
> The elimination of getObjectClass() seems like a good end goal IMO, so
> the direction of the patch is interesting.  Would object_type_map and
> ObjectProperty follow the same idea of relying on the catalogs OID
> instead of the OBJECT_*?
>
> Note that there are still two dependencies to getObjectClass() in
> event_trigger.c and dependency.c.
> --

I refactored dependency.c, event_trigger.c based on
0001-Remove-ObjectClass.patch.
dependency.c already includes a bunch of catalog header files, but
event_trigger.c doesn't.
Now we need to "include" around 30 header files in event_trigger.c,
not sure if it's ok or not.

0001-Remove-ObjectClass.patch
We also need to refactor getObjectIdentityParts's below comments?
/*
* There's intentionally no default: case here; we want the
* compiler to warn if a new OCLASS hasn't been handled above.
*/
since OCLASS is removed.

`bool EventTriggerSupportsObjectClass(ObjectClass objclass)`
change to
`bool EventTriggerSupportsObjectClass(Oid classId)`

I think the function name should also be refactored.
I'm not sure of the new function name, so I didn't change.

  29. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-01T09:26:45Z

    On 01.03.24 05:08, Michael Paquier wrote:
> On Thu, Feb 29, 2024 at 12:41:38PM +0100, Peter Eisentraut wrote:
>> On 27.02.24 08:57, Alvaro Herrera wrote:
>>> On 2024-Feb-27, Michael Paquier wrote:
>>>> These would cause compilation failures.  Saying that, this is a very
>>>> nice cleanup, so I've fixed these and applied the patch after checking
>>>> that the one-one replacements were correct.
>>>
>>> Oh, I thought we were going to get rid of ObjectClass altogether -- I
>>> mean, have getObjectClass() return ObjectAddress->classId, and then
>>> define the OCLASS values for each catalog OID [... tries to ...]  But
>>> this(*) doesn't work for two reasons:
>>
>> I have long wondered what the point of ObjectClass is.  I find the extra
>> layer of redirection, which is used only in small parts of the code, and the
>> similarity to ObjectType confusing.  I happened to have a draft patch for
>> its removal lying around, so I'll show it here, rebased over what has
>> already been done in this thread.
> 
> The elimination of getObjectClass() seems like a good end goal IMO, so
> the direction of the patch is interesting.  Would object_type_map and
> ObjectProperty follow the same idea of relying on the catalogs OID
> instead of the OBJECT_*?
> 
> Note that there are still two dependencies to getObjectClass() in
> event_trigger.c and dependency.c.

Oops, there was a second commit in my branch that I neglected to send 
in.  Here is my complete patch set.

  30. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-03T23:46:17Z

    On Fri, Mar 01, 2024 at 06:30:10AM +0100, Jelte Fennema-Nio wrote:
> On Fri, 1 Mar 2024 at 06:08, Michael Paquier <michael@paquier.xyz> wrote:
>> Mostly OK to me.  Just note that this comment is incorrect because
>> pg_enc2gettext_tbl[] includes elements in the range
>> [PG_SJIS,_PG_LAST_ENCODING_[  ;)
> 
> fixed

(Forgot to update this thread.)
Thanks, applied this one.  I went over a few versions of the comment
in pg_wchar.h, and tweaked it to something that was of one of the
previous versions, I think.  
--
Michael

  31. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-03-04T01:29:03Z

    On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> Oops, there was a second commit in my branch that I neglected to send
> in.  Here is my complete patch set.

there is a `OCLASS` at the end of getObjectIdentityParts.
there is a `ObjectClass` in typedefs.list

  32. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-03-05T13:50:05Z

    On Mon, 04 Mar 2024 at 07:46, Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Mar 01, 2024 at 06:30:10AM +0100, Jelte Fennema-Nio wrote:
>> On Fri, 1 Mar 2024 at 06:08, Michael Paquier <michael@paquier.xyz> wrote:
>>> Mostly OK to me.  Just note that this comment is incorrect because
>>> pg_enc2gettext_tbl[] includes elements in the range
>>> [PG_SJIS,_PG_LAST_ENCODING_[  ;)
>>
>> fixed
>
> (Forgot to update this thread.)
> Thanks, applied this one.  I went over a few versions of the comment
> in pg_wchar.h, and tweaked it to something that was of one of the
> previous versions, I think.

Hi,

Attach a patch to rewrite dispatch_table array using C99-designated
initializer syntax.

  33. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-05T14:03:54Z

    On Tue, 5 Mar 2024 at 14:50, Japin Li <japinli@hotmail.com> wrote:
> Attach a patch to rewrite dispatch_table array using C99-designated
> initializer syntax.

Looks good. Two small things:

+       [EEOP_LAST] = &&CASE_EEOP_LAST,

Is EEOP_LAST actually needed in this array? It seems unused afaict. If
indeed not needed, that would be good to remove in an additional
commit.

- *
- * The order of entries needs to be kept in sync with the dispatch_table[]
- * array in execExprInterp.c:ExecInterpExpr().

I think it would be good to at least keep the comment saying that this
array should be updated (only the order doesn't need to be strictly
kept in sync anymore).

  34. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-03-05T14:30:34Z

    On Tue, 05 Mar 2024 at 22:03, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Tue, 5 Mar 2024 at 14:50, Japin Li <japinli@hotmail.com> wrote:
>> Attach a patch to rewrite dispatch_table array using C99-designated
>> initializer syntax.
>
> Looks good. Two small things:

Thanks for the review.

>
> +       [EEOP_LAST] = &&CASE_EEOP_LAST,
>
> Is EEOP_LAST actually needed in this array? It seems unused afaict. If
> indeed not needed, that would be good to remove in an additional
> commit.

There is a warning if remove it, so I keep it.

/home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:118:33: warning: label ‘CASE_EEOP_LAST’ defined but not used [-Wunused-label]
  118 | #define EEO_CASE(name)          CASE_##name:
      |                                 ^~~~~
/home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:1845:17: note: in expansion of macro ‘EEO_CASE’
 1845 |                 EEO_CASE(EEOP_LAST)
      |                 ^~~~~~~~

>
> - *
> - * The order of entries needs to be kept in sync with the dispatch_table[]
> - * array in execExprInterp.c:ExecInterpExpr().
>
> I think it would be good to at least keep the comment saying that this
> array should be updated (only the order doesn't need to be strictly
> kept in sync anymore).

Fixed.

  35. Re: Improve readability by using designated initializers when possible

    Jelte Fennema-Nio <postgres@jeltef.nl> — 2024-03-05T17:53:16Z

    On Tue, 5 Mar 2024 at 15:30, Japin Li <japinli@hotmail.com> wrote:
> There is a warning if remove it, so I keep it.
>
> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:118:33: warning: label ‘CASE_EEOP_LAST’ defined but not used [-Wunused-label]
>   118 | #define EEO_CASE(name)          CASE_##name:
>       |                                 ^~~~~
> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:1845:17: note: in expansion of macro ‘EEO_CASE’
>  1845 |                 EEO_CASE(EEOP_LAST)
>       |                 ^~~~~~~~

I think if you remove the EEO_CASE(EEOP_LAST) block the warning should
go away. That block is clearly marked as unreachable, so it doesn't
really serve a purpose.

  36. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-03-06T00:24:09Z

    On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
> On Tue, 5 Mar 2024 at 15:30, Japin Li <japinli@hotmail.com> wrote:
>> There is a warning if remove it, so I keep it.
>>
>> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:118:33: warning: label ‘CASE_EEOP_LAST’ defined but not used [-Wunused-label]
>>   118 | #define EEO_CASE(name)          CASE_##name:
>>       |                                 ^~~~~
>> /home/japin/Codes/postgres/build/../src/backend/executor/execExprInterp.c:1845:17: note: in expansion of macro ‘EEO_CASE’
>>  1845 |                 EEO_CASE(EEOP_LAST)
>>       |                 ^~~~~~~~
>
> I think if you remove the EEO_CASE(EEOP_LAST) block the warning should
> go away. That block is clearly marked as unreachable, so it doesn't
> really serve a purpose.

Thanks! Fixed as you suggested.  Please see v3 patch.

  37. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-08T05:21:05Z

    On Wed, Mar 06, 2024 at 08:24:09AM +0800, Japin Li wrote:
> On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>> I think if you remove the EEO_CASE(EEOP_LAST) block the warning should
>> go away. That block is clearly marked as unreachable, so it doesn't
>> really serve a purpose.
> 
> Thanks! Fixed as you suggested.  Please see v3 patch.

Hmm.  I am not sure if this one is a good idea.  This makes the code a
bit more complicated to grasp under EEO_USE_COMPUTED_GOTO with the
reverse dispatch table, to say the least.
--
Michael

  38. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-08T05:50:45Z

    On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote:
> On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>> Oops, there was a second commit in my branch that I neglected to send
>> in.  Here is my complete patch set.

Thanks for the new patch set.  The gains are neat, giving nice
numbers:
 7 files changed, 200 insertions(+), 644 deletions(-)

+       default:
+           DropObjectById(object);
+           break;

Hmm.  I am not sure that this is a good idea.  Wouldn't it be safer to
use as default path something that generates an ERROR so as this code
path would complain immediately when adding a new catalog?  My point
is to make people consider what they should do on deletion when adding
a catalog that would take this code path, rather than assuming that a
deletion is OK to happen.  So I would recommend to keep the list of
catalog OIDs for the DropObjectById case, keep the list for global
objects, and add a third path with a new ERROR.

-           /*
-            * There's intentionally no default: case here; we want the
-            * compiler to warn if a new OCLASS hasn't been handled above.
-            */

In getObjectDescription() and getObjectTypeDescription() this was a
safeguard, but we don't have that anymore.  So it seems to me that
this should be replaced with a default with elog(ERROR)?

There is a third one in getObjectIdentityParts() that has not been
removed, though, but same remark at the two others.

RememberAllDependentForRebuilding() uses a default, so this one looks
good to me.

> there is a `OCLASS` at the end of getObjectIdentityParts.

Nice catch.  A comment is not updated.

> There is a `ObjectClass` in typedefs.list

This is usually taken care of by committers or updated automatically.
--
Michael

  39. Re: Improve readability by using designated initializers when possible

    Japin Li <japinli@hotmail.com> — 2024-03-12T01:28:32Z

    On Fri, 08 Mar 2024 at 13:21, Michael Paquier <michael@paquier.xyz> wrote:
> On Wed, Mar 06, 2024 at 08:24:09AM +0800, Japin Li wrote:
>> On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio <postgres@jeltef.nl> wrote:
>>> I think if you remove the EEO_CASE(EEOP_LAST) block the warning should
>>> go away. That block is clearly marked as unreachable, so it doesn't
>>> really serve a purpose.
>>
>> Thanks! Fixed as you suggested.  Please see v3 patch.
>
> Hmm.  I am not sure if this one is a good idea.

Sorry for the late reply!

> This makes the code a
> bit more complicated to grasp under EEO_USE_COMPUTED_GOTO with the
> reverse dispatch table, to say the least.

I'm not get your mind.  Could you explain in more detail?

  40. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-13T13:24:32Z

    On 08.03.24 06:50, Michael Paquier wrote:
> On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote:
>> On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>>> Oops, there was a second commit in my branch that I neglected to send
>>> in.  Here is my complete patch set.
> 
> Thanks for the new patch set.  The gains are neat, giving nice
> numbers:
>   7 files changed, 200 insertions(+), 644 deletions(-)
> 
> +       default:
> +           DropObjectById(object);
> +           break;
> 
> Hmm.  I am not sure that this is a good idea.  Wouldn't it be safer to
> use as default path something that generates an ERROR so as this code
> path would complain immediately when adding a new catalog?

fixed in new patch

> In getObjectDescription() and getObjectTypeDescription() this was a
> safeguard, but we don't have that anymore.  So it seems to me that
> this should be replaced with a default with elog(ERROR)?

fixed

>> there is a `OCLASS` at the end of getObjectIdentityParts.
> 
> Nice catch.  A comment is not updated.
> 
>> There is a `ObjectClass` in typedefs.list
> 
> This is usually taken care of by committers or updated automatically.

both fixed

  41. Re: Improve readability by using designated initializers when possible

    Michael Paquier <michael@paquier.xyz> — 2024-03-14T00:26:25Z

    On Wed, Mar 13, 2024 at 02:24:32PM +0100, Peter Eisentraut wrote:
> On 08.03.24 06:50, Michael Paquier wrote:
>> This is usually taken care of by committers or updated automatically.
> 
> both fixed

Looks mostly fine, thanks for the new version.

-EventTriggerSupportsObjectClass(ObjectClass objclass)
+EventTriggerSupportsObject(const ObjectAddress *object) 

The shortcut introduced here is interesting, but it is inconsistent.
HEAD treats OCLASS_SUBSCRIPTION as something supported by event
triggers, but as pg_subscription is a shared catalog it would be
discarded with your change.  Subscriptions are marked as supported in
the event trigger table:
https://www.postgresql.org/docs/devel/event-trigger-matrix.html
--
Michael

  42. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-18T07:09:14Z

    On 14.03.24 01:26, Michael Paquier wrote:
> -EventTriggerSupportsObjectClass(ObjectClass objclass)
> +EventTriggerSupportsObject(const ObjectAddress *object)
> 
> The shortcut introduced here is interesting, but it is inconsistent.
> HEAD treats OCLASS_SUBSCRIPTION as something supported by event
> triggers, but as pg_subscription is a shared catalog it would be
> discarded with your change.  Subscriptions are marked as supported in
> the event trigger table:
> https://www.postgresql.org/docs/devel/event-trigger-matrix.html

Ah, good catch.  Subscriptions are a little special there.  Here is a 
new patch that keeps the switch/case arrangement in that function.  That 
also makes it easier to keep the two EventTriggerSupports... functions 
aligned.  Also added a note about subscriptions and a reference to the 
documentation.

  43. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-03-18T10:01:20Z

    On Mon, Mar 18, 2024 at 3:09 PM Peter Eisentraut <peter@eisentraut.org> wrote:
>
> On 14.03.24 01:26, Michael Paquier wrote:
> > -EventTriggerSupportsObjectClass(ObjectClass objclass)
> > +EventTriggerSupportsObject(const ObjectAddress *object)
> >
> > The shortcut introduced here is interesting, but it is inconsistent.
> > HEAD treats OCLASS_SUBSCRIPTION as something supported by event
> > triggers, but as pg_subscription is a shared catalog it would be
> > discarded with your change.  Subscriptions are marked as supported in
> > the event trigger table:
> > https://www.postgresql.org/docs/devel/event-trigger-matrix.html
>
> Ah, good catch.  Subscriptions are a little special there.  Here is a
> new patch that keeps the switch/case arrangement in that function.  That
> also makes it easier to keep the two EventTriggerSupports... functions
> aligned.  Also added a note about subscriptions and a reference to the
> documentation.

select relname from pg_class where relisshared and relkind = 'r';
        relname
-----------------------
 pg_authid
 pg_subscription
 pg_database
 pg_db_role_setting
 pg_tablespace
 pg_auth_members
 pg_shdepend
 pg_shdescription
 pg_replication_origin
 pg_shseclabel
 pg_parameter_acl
(11 rows)

EventTriggerSupportsObject should return false for the following:
SharedSecLabelRelationId
SharedDescriptionRelationId
DbRoleSettingRelationId
SharedDependRelationId

but I am not sure ReplicationOriginRelationId.

  44. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-03-18T12:58:19Z

    On Mon, Mar 18, 2024 at 6:01 PM jian he <jian.universality@gmail.com> wrote:
>
> On Mon, Mar 18, 2024 at 3:09 PM Peter Eisentraut <peter@eisentraut.org> wrote:
> >
> > On 14.03.24 01:26, Michael Paquier wrote:
> > > -EventTriggerSupportsObjectClass(ObjectClass objclass)
> > > +EventTriggerSupportsObject(const ObjectAddress *object)
> > >
> > > The shortcut introduced here is interesting, but it is inconsistent.
> > > HEAD treats OCLASS_SUBSCRIPTION as something supported by event
> > > triggers, but as pg_subscription is a shared catalog it would be
> > > discarded with your change.  Subscriptions are marked as supported in
> > > the event trigger table:
> > > https://www.postgresql.org/docs/devel/event-trigger-matrix.html
> >
> > Ah, good catch.  Subscriptions are a little special there.  Here is a
> > new patch that keeps the switch/case arrangement in that function.  That
> > also makes it easier to keep the two EventTriggerSupports... functions
> > aligned.  Also added a note about subscriptions and a reference to the
> > documentation.
>
> select relname from pg_class where relisshared and relkind = 'r';
>         relname
> -----------------------
>  pg_authid
>  pg_subscription
>  pg_database
>  pg_db_role_setting
>  pg_tablespace
>  pg_auth_members
>  pg_shdepend
>  pg_shdescription
>  pg_replication_origin
>  pg_shseclabel
>  pg_parameter_acl
> (11 rows)
>

also in function doDeletion
we have:

/*
* These global object types are not supported here.
*/
case AuthIdRelationId:
case DatabaseRelationId:
case TableSpaceRelationId:
case SubscriptionRelationId:
case ParameterAclRelationId:
elog(ERROR, "global objects cannot be deleted by doDeletion");
break;

do we need to add other global objects?

in the end, it does not matter since we have:
default:
elog(ERROR, "unsupported object class: %u", object->classId);

  45. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-20T14:08:39Z

    On 18.03.24 11:01, jian he wrote:
> select relname from pg_class where relisshared and relkind = 'r';
>          relname
> -----------------------
>   pg_authid
>   pg_subscription
>   pg_database
>   pg_db_role_setting
>   pg_tablespace
>   pg_auth_members
>   pg_shdepend
>   pg_shdescription
>   pg_replication_origin
>   pg_shseclabel
>   pg_parameter_acl
> (11 rows)
> 
> EventTriggerSupportsObject should return false for the following:
> SharedSecLabelRelationId
> SharedDescriptionRelationId
> DbRoleSettingRelationId
> SharedDependRelationId
> 
> but I am not sure ReplicationOriginRelationId.

EventTriggerSupportsObject() (currently named 
EventTriggerSupportsObjectClass()) is only used by the deletion code, 
and these additional classes are not supported there anyway.  Also, if 
they happen to show up there for some reason, then 
EventTriggerSQLDropAddObject() would error out in 
getObjectIdentityParts() or getObjectTypeDescription().  So you wouldn't 
get an event trigger firing on a previously unsupported class by 
accident.  So I think this is robust enough.

  46. Re: Improve readability by using designated initializers when possible

    jian he <jian.universality@gmail.com> — 2024-03-25T05:00:00Z

    looking through v4 again.
v4 looks good to me.

  47. Re: Improve readability by using designated initializers when possible

    Peter Eisentraut <peter@eisentraut.org> — 2024-03-26T10:15:22Z

    On 25.03.24 06:00, jian he wrote:
> looking through v4 again.
> v4 looks good to me.

Thanks, I have committed this.