Improve readability by using designated initializers when possible

On Wed, 2024-02-21 at 16:03 +0100, Jelte Fennema-Nio wrote:

Usage of designated initializers came up in:
/messages/by-id/ZdWXhAt9Tz4d-lut@paquier.xyz

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

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.

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

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.

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.

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]/messages/by-id/386f8c45-c8ac-4681-8add-e3b0852c1620@eisentraut.org.

[1]: /messages/by-id/386f8c45-c8ac-4681-8add-e3b0852c1620@eisentraut.org

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] /messages/by-id/386f8c45-c8ac-4681-8add-e3b0852c1620@eisentraut.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

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

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)

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 :)

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.

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).

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

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");

/*

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).

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

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

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).

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