Обсуждение: Improve readability by using designated initializers when possible
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.
Вложения
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
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].
[1] https://www.postgresql.org/message-id/386f8c45-c8ac-4681-8add-e3b0852c1620%40eisentraut.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] 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
Вложения
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
Вложения
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.
Вложения
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.
Вложения
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
Вложения
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
Вложения
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.
Вложения
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
Вложения
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
Вложения
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.
Вложения
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.
Вложения
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
Вложения
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
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.
Вложения
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).
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’
definedbut 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.
Вложения
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’ definedbut 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.
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’ definedbut 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.
Вложения
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
Вложения
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
Вложения
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?
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
Вложения
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
Вложения
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.
Вложения
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.
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);
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.
looking through v4 again. v4 looks good to me.
On 25.03.24 06:00, jian he wrote: > looking through v4 again. > v4 looks good to me. Thanks, I have committed this.