Re: Improve readability by using designated initializers when possible
On 25.03.24 06:00, jian he wrote: looking through v4 again. v4 looks good to me. Thanks, I have committed this.
Re: Improve readability by using designated initializers when possible
looking through v4 again. v4 looks good to me.
Re: Improve readability by using designated initializers when possible
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.
Re: Improve readability by using designated initializers when possible
On Mon, Mar 18, 2024 at 6:01 PM jian he wrote: > > On Mon, Mar 18, 2024 at 3:09 PM Peter Eisentraut 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);
Re: Improve readability by using designated initializers when possible
On Mon, Mar 18, 2024 at 3:09 PM Peter Eisentraut 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.
Re: Improve readability by using designated initializers when possible
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.
From 3a17f075f9ccac2eb7a969be5e455bb29a9e40e2 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 18 Mar 2024 08:07:37 +0100
Subject: [PATCH v4] Remove ObjectClass
ObjectClass is an enum whose values correspond to catalog OIDs. But
the extra layer of redirection, which is used only in small parts of
the code, and the similarity to ObjectType, are confusing and
cumbersome.
One advantage has been that 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. But 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.
Discussion:
https://www.postgresql.org/message-id/flat/CAGECzQT3caUbcCcszNewCCmMbCuyP7XNAm60J3ybd6PN5kH2Dw%40mail.gmail.com
---
src/backend/catalog/dependency.c | 239
src/backend/catalog/objectaddress.c | 316 ---
src/backend/commands/alter.c | 73 ++-
src/backend/commands/event_trigger.c | 130 ++-
src/backend/commands/tablecmds.c | 62 ++
src/include/catalog/dependency.h | 51 -
src/include/commands/event_trigger.h | 2 +-
src/tools/pgindent/typedefs.list | 1 -
8 files changed, 233 insertions(+), 641 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index eadcf6af0df..6e8f6a57051 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -206,7 +206,7 @@ deleteObjectsInList(ObjectAddresses *targetObjects,
Relation *depRel,
if (extra->flags & DEPFLAG_REVERSE)
normal = true;
- if
(EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
+ if (EventTriggerSupportsObject(thisobj))
{
EventTriggerSQLDropAddObject(thisobj, original,
normal);
}
@@ -1349,9 +1349,9 @@ deleteOneObject(const ObjectAddress *object, Relation
*depRel, int flags)
static void
doDeletion(const ObjectAddress *object, int flags)
{
- switch (getObjectClass(object))
+ switch (object->classId)
{
- case OCLASS_CLASS:
+ case RelationRelationId:
{
charrelKind =
get_rel_relkind(object->objectId);
@@ -1382,104 +1382,102 @@ doDeletion(const ObjectAddress *object, int flags)
break;
}
- case OCLASS_PROC:
+ case ProcedureRelationId:
RemoveFunctionById(object->objectId);
break;
- case OCLASS_TYPE:
+ case TypeRelationId:
RemoveTypeById(object->objectId);
break;
- case OCLASS_CONSTRAINT:
+ case ConstraintRelationId:
RemoveConstraintById(object->objectId);
break;
- case OCLASS_DEFAULT:
+ case AttrDefaultRelationId:
RemoveAttrDefaultById(object->objectId);
break;
- case OCLASS_LARGEOBJECT:
+ case LargeObjectRelationId:
LargeObjectDrop(object->objectId);
break;
- case OCLASS_OPERATOR:
+ case OperatorRelationId:
RemoveOperatorById(object->objectId);
break;
- case OCLASS_REWRITE:
+ case RewriteRelationId:
RemoveRewriteRuleById(object->objectId);
break;
-
Re: Improve readability by using designated initializers when possible
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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
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 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
From a40a97de94d69d44b3a04b7987e99c8a6675bccf Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Wed, 13 Mar 2024 14:16:09 +0100
Subject: [PATCH v3] Remove ObjectClass
ObjectClass is an enum whose values correspond to catalog OIDs. But
the extra layer of redirection, which is used only in small parts of
the code, and the similarity to ObjectType, are confusing and
cumbersome.
One advantage has been that 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. But 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.
Discussion:
https://www.postgresql.org/message-id/flat/CAGECzQT3caUbcCcszNewCCmMbCuyP7XNAm60J3ybd6PN5kH2Dw%40mail.gmail.com
---
src/backend/catalog/dependency.c | 239
src/backend/catalog/objectaddress.c | 316 ---
src/backend/commands/alter.c | 73 ++-
src/backend/commands/event_trigger.c | 122 +--
src/backend/commands/tablecmds.c | 62 ++
src/include/catalog/dependency.h | 51 -
src/include/commands/event_trigger.h | 2 +-
src/tools/pgindent/typedefs.list | 1 -
8 files changed, 222 insertions(+), 644 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index eadcf6af0df..6e8f6a57051 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -206,7 +206,7 @@ deleteObjectsInList(ObjectAddresses *targetObjects,
Relation *depRel,
if (extra->flags & DEPFLAG_REVERSE)
normal = true;
- if
(EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
+ if (EventTriggerSupportsObject(thisobj))
{
EventTriggerSQLDropAddObject(thisobj, original,
normal);
}
@@ -1349,9 +1349,9 @@ deleteOneObject(const ObjectAddress *object, Relation
*depRel, int flags)
static void
doDeletion(const ObjectAddress *object, int flags)
{
- switch (getObjectClass(object))
+ switch (object->classId)
{
- case OCLASS_CLASS:
+ case RelationRelationId:
{
charrelKind =
get_rel_relkind(object->objectId);
@@ -1382,104 +1382,102 @@ doDeletion(const ObjectAddress *object, int flags)
break;
}
- case OCLASS_PROC:
+ case ProcedureRelationId:
RemoveFunctionById(object->objectId);
break;
- case OCLASS_TYPE:
+ case TypeRelationId:
RemoveTypeById(object->objectId);
break;
- case OCLASS_CONSTRAINT:
+ case ConstraintRelationId:
RemoveConstraintById(object->objectId);
break;
- case OCLASS_DEFAULT:
+ case AttrDefaultRelationId:
RemoveAttrDefaultById(object->objectId);
break;
- case OCLASS_LARGEOBJECT:
+ case LargeObjectRelationId:
LargeObjectDrop(object->objectId);
break;
- case OCLASS_OPERATOR:
+ case
Re: Improve readability by using designated initializers when possible
On Fri, 08 Mar 2024 at 13:21, Michael Paquier 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 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?
Re: Improve readability by using designated initializers when possible
On Mon, Mar 04, 2024 at 09:29:03AM +0800, jian he wrote: > On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut 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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Wed, Mar 06, 2024 at 08:24:09AM +0800, Japin Li wrote: > On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio 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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Wed, 06 Mar 2024 at 01:53, Jelte Fennema-Nio wrote:
> On Tue, 5 Mar 2024 at 15:30, Japin Li 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.
>From ff4d9ce75cfd35a1865bbfb9cd5664a7806d92be Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v3 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 203 --
src/include/executor/execExpr.h | 2 +-
2 files changed, 97 insertions(+), 108 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..df9fe79058 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,110 +400,106 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_BYREF,
- &_EEOP_AGG_PRESORTED_DISTINCT_SINGLE,
- &_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
- &_EEOP_AGG_ORDERED_TRANS_DATUM,
- &_EEOP_AGG_ORDERED_TRANS_TUPLE,
- &_EEOP_LAST
+ [EEOP_DONE] = &_EEOP_DONE,
+ [EEOP_INNER_FETCHSOME] = &_EEOP_INNER_FETCHSOME,
+ [EEOP_OUTER_FETCHSOME] = &_EEOP_OUTER_FETCHSOME,
+ [EEOP_SCAN_FETCHSOME] = &_EEOP_SCAN_FETCHSOME,
+ [EEOP_INNER_VAR] = &_EEOP_INNER_VAR,
+ [EEOP_OUTER_VAR] = &_EEOP_OUTER_VAR,
+ [EEOP_SCAN_VAR] = &_EEOP_SCAN_VAR,
+ [EEOP_INNER_SYSVAR] = &_EEOP_INNER_SYSVAR,
+ [EEOP_OUTER_SYSVAR] = &_EEOP_OUTER_SYSVAR,
+ [EEOP_SCAN_SYSVAR] = &_EEOP_SCAN_SYSVAR,
+ [EEOP_WHOLEROW] = &_EEOP_WHOLEROW,
+ [EEOP_ASSIGN_INNER_VAR] = &_EEOP_ASSIGN_INNER_VAR,
+ [EEOP_ASSIGN_OUTER_VAR] = &_EEOP_ASSIGN_OUTER_VAR,
+ [EEOP_ASSIGN_SCAN_VAR] = &_EEOP_ASSIGN_SCAN_VAR,
+
Re: Improve readability by using designated initializers when possible
On Tue, 5 Mar 2024 at 15:30, Japin Li 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.
Re: Improve readability by using designated initializers when possible
On Tue, 05 Mar 2024 at 22:03, Jelte Fennema-Nio wrote:
> On Tue, 5 Mar 2024 at 14:50, Japin Li 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] = &_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.
>From 20a72f2a5e0b282812ecde5c6aef2ce4ab118003 Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v2 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 195 +-
src/include/executor/execExpr.h | 2 +-
2 files changed, 97 insertions(+), 100 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..e4ad522312 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,107 +400,104 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_BYREF,
- &_EEOP_AGG_PRESORTED_DISTINCT_SINGLE,
- &_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
- &_EEOP_AGG_ORDERED_TRANS_DATUM,
- &_EEOP_AGG_ORDERED_TRANS_TUPLE,
- &_EEOP_LAST
+ [EEOP_DONE] = &_EEOP_DONE,
+ [EEOP_INNER_FETCHSOME] = &_EEOP_INNER_FETCHSOME,
+ [EEOP_OUTER_FETCHSOME] = &_EEOP_OUTER_FETCHSOME,
+ [EEOP_SCAN_FETCHSOME] = &_EEOP_SCAN_FETCHSOME,
+ [EEOP_INNER_VAR] = &_EEOP_INNER_VAR,
+ [EEOP_OUTER_VAR] =
Re: Improve readability by using designated initializers when possible
On Tue, 5 Mar 2024 at 14:50, Japin Li wrote: > Attach a patch to rewrite dispatch_table array using C99-designated > initializer syntax. Looks good. Two small things: + [EEOP_LAST] = &_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).
Re: Improve readability by using designated initializers when possible
On Mon, 04 Mar 2024 at 07:46, Michael Paquier 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 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.
>From f398d6d310e9436c5e415baa6fd273981a9e181f Mon Sep 17 00:00:00 2001
From: Japin Li
Date: Tue, 5 Mar 2024 21:32:46 +0800
Subject: [PATCH v1 1/1] Use C99-designated initializer syntax for
dispatch_table array
---
src/backend/executor/execExprInterp.c | 195 +-
src/include/executor/execExpr.h | 3 -
2 files changed, 96 insertions(+), 102 deletions(-)
diff --git a/src/backend/executor/execExprInterp.c b/src/backend/executor/execExprInterp.c
index 7c1f51e2e0..e4ad522312 100644
--- a/src/backend/executor/execExprInterp.c
+++ b/src/backend/executor/execExprInterp.c
@@ -400,107 +400,104 @@ ExecInterpExpr(ExprState *state, ExprContext *econtext, bool *isnull)
TupleTableSlot *outerslot;
TupleTableSlot *scanslot;
- /*
- * This array has to be in the same order as enum ExprEvalOp.
- */
#if defined(EEO_USE_COMPUTED_GOTO)
static const void *const dispatch_table[] = {
- &_EEOP_DONE,
- &_EEOP_INNER_FETCHSOME,
- &_EEOP_OUTER_FETCHSOME,
- &_EEOP_SCAN_FETCHSOME,
- &_EEOP_INNER_VAR,
- &_EEOP_OUTER_VAR,
- &_EEOP_SCAN_VAR,
- &_EEOP_INNER_SYSVAR,
- &_EEOP_OUTER_SYSVAR,
- &_EEOP_SCAN_SYSVAR,
- &_EEOP_WHOLEROW,
- &_EEOP_ASSIGN_INNER_VAR,
- &_EEOP_ASSIGN_OUTER_VAR,
- &_EEOP_ASSIGN_SCAN_VAR,
- &_EEOP_ASSIGN_TMP,
- &_EEOP_ASSIGN_TMP_MAKE_RO,
- &_EEOP_CONST,
- &_EEOP_FUNCEXPR,
- &_EEOP_FUNCEXPR_STRICT,
- &_EEOP_FUNCEXPR_FUSAGE,
- &_EEOP_FUNCEXPR_STRICT_FUSAGE,
- &_EEOP_BOOL_AND_STEP_FIRST,
- &_EEOP_BOOL_AND_STEP,
- &_EEOP_BOOL_AND_STEP_LAST,
- &_EEOP_BOOL_OR_STEP_FIRST,
- &_EEOP_BOOL_OR_STEP,
- &_EEOP_BOOL_OR_STEP_LAST,
- &_EEOP_BOOL_NOT_STEP,
- &_EEOP_QUAL,
- &_EEOP_JUMP,
- &_EEOP_JUMP_IF_NULL,
- &_EEOP_JUMP_IF_NOT_NULL,
- &_EEOP_JUMP_IF_NOT_TRUE,
- &_EEOP_NULLTEST_ISNULL,
- &_EEOP_NULLTEST_ISNOTNULL,
- &_EEOP_NULLTEST_ROWISNULL,
- &_EEOP_NULLTEST_ROWISNOTNULL,
- &_EEOP_BOOLTEST_IS_TRUE,
- &_EEOP_BOOLTEST_IS_NOT_TRUE,
- &_EEOP_BOOLTEST_IS_FALSE,
- &_EEOP_BOOLTEST_IS_NOT_FALSE,
- &_EEOP_PARAM_EXEC,
- &_EEOP_PARAM_EXTERN,
- &_EEOP_PARAM_CALLBACK,
- &_EEOP_CASE_TESTVAL,
- &_EEOP_MAKE_READONLY,
- &_EEOP_IOCOERCE,
- &_EEOP_IOCOERCE_SAFE,
- &_EEOP_DISTINCT,
- &_EEOP_NOT_DISTINCT,
- &_EEOP_NULLIF,
- &_EEOP_SQLVALUEFUNCTION,
- &_EEOP_CURRENTOFEXPR,
- &_EEOP_NEXTVALUEEXPR,
- &_EEOP_ARRAYEXPR,
- &_EEOP_ARRAYCOERCE,
- &_EEOP_ROW,
- &_EEOP_ROWCOMPARE_STEP,
- &_EEOP_ROWCOMPARE_FINAL,
- &_EEOP_MINMAX,
- &_EEOP_FIELDSELECT,
- &_EEOP_FIELDSTORE_DEFORM,
- &_EEOP_FIELDSTORE_FORM,
- &_EEOP_SBSREF_SUBSCRIPTS,
- &_EEOP_SBSREF_OLD,
- &_EEOP_SBSREF_ASSIGN,
- &_EEOP_SBSREF_FETCH,
- &_EEOP_DOMAIN_TESTVAL,
- &_EEOP_DOMAIN_NOTNULL,
- &_EEOP_DOMAIN_CHECK,
- &_EEOP_CONVERT_ROWTYPE,
- &_EEOP_SCALARARRAYOP,
- &_EEOP_HASHED_SCALARARRAYOP,
- &_EEOP_XMLEXPR,
- &_EEOP_JSON_CONSTRUCTOR,
- &_EEOP_IS_JSON,
- &_EEOP_AGGREF,
- &_EEOP_GROUPING_FUNC,
- &_EEOP_WINDOW_FUNC,
- &_EEOP_SUBPLAN,
- &_EEOP_AGG_STRICT_DESERIALIZE,
- &_EEOP_AGG_DESERIALIZE,
- &_EEOP_AGG_STRICT_INPUT_CHECK_ARGS,
- &_EEOP_AGG_STRICT_INPUT_CHECK_NULLS,
- &_EEOP_AGG_PLAIN_PERGROUP_NULLCHECK,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_BYVAL,
- &_EEOP_AGG_PLAIN_TRANS_INIT_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_STRICT_BYREF,
- &_EEOP_AGG_PLAIN_TRANS_BYREF,
- &_EEOP_AGG_PRESORTED_DISTINCT_SINGLE,
- &_EEOP_AGG_PRESORTED_DISTINCT_MULTI,
- &_EEOP_AGG_ORDERED_TRANS_DATUM,
- &_EEOP_AGG_ORDERED_TRANS_TUPLE,
- &_EEOP_LAST
+ [EEOP_DONE] = &_EEOP_DONE,
+ [EEOP_INNER_FETCHSOME] = &_EEOP_INNER_FETCHSOME,
+ [EEOP_OUTER_FETCHSOME] = &_EEOP_OUTER_FETCHSOME,
+ [EEOP_SCAN_FETCHSOME] = &_EEOP_SCAN_FETCHSOME,
+ [EEOP_INNER_VAR] = &_EEOP_INNER_VAR,
+ [EEOP_OUTER_VAR] = &_EEOP_OUTER_VAR,
+ [EEOP_SCAN_VAR] = &_EEOP_SCAN_VAR,
+ [EEOP_INNER_SYSVAR] = &_EEOP_INNER_SYSVAR,
+ [EEOP_OUTER_SYSVAR] = &_EEOP_OUTER_SYSVAR,
+ [EEOP_SCAN_SYSVAR] = &_EEOP_SCAN_SYSVAR,
+ [EEOP_WHOLEROW] = &_EEOP_WHOLEROW,
+ [EEOP_ASSIGN_INNER_VAR] = &_EEOP_ASSIGN_INNER_VAR,
+ [EEOP_ASSIGN_OUTER_VAR] = &_EEOP_ASSIGN_OUTER_VAR,
+ [EEOP_ASSIGN_SCAN_VAR] = &_EEOP_ASSIGN_SCAN_VAR,
+ [EEOP_ASSIGN_TMP] = &_EEOP_ASSIGN_TMP,
+ [EEOP_ASSIGN_TMP_MAKE_RO] = &_EEOP_ASSIGN_TMP_MAKE_RO,
+ [EEOP_CONST] = &_EEOP_CONST,
+ [EEOP_FUNCEXPR] = &_EEOP_FUNCEXPR,
+ [EEOP_FUNCEXPR_STRICT] = &_EEOP_FUNCEXPR_STRICT,
Re: Improve readability by using designated initializers when possible
On Fri, Mar 1, 2024 at 5:26 PM Peter Eisentraut 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
Re: Improve readability by using designated initializers when possible
On Fri, Mar 01, 2024 at 06:30:10AM +0100, Jelte Fennema-Nio wrote: > On Fri, 1 Mar 2024 at 06:08, Michael Paquier 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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
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.
From 00fe9c6163261c1fdfc806934903bcd60b857633 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Sun, 11 Sep 2022 22:26:55 +0200
Subject: [PATCH v2 1/2] Don't use ObjectClass for event trigger support
---
src/backend/catalog/dependency.c | 2 +-
src/backend/commands/event_trigger.c | 122 +++
src/include/commands/event_trigger.h | 2 +-
3 files changed, 12 insertions(+), 114 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 2eb41d537b..192dedb3cb 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -208,7 +208,7 @@ deleteObjectsInList(ObjectAddresses *targetObjects,
Relation *depRel,
if (extra->flags & DEPFLAG_REVERSE)
normal = true;
- if
(EventTriggerSupportsObjectClass(getObjectClass(thisobj)))
+ if (EventTriggerSupportsObject(thisobj))
{
EventTriggerSQLDropAddObject(thisobj, original,
normal);
}
diff --git a/src/backend/commands/event_trigger.c
b/src/backend/commands/event_trigger.c
index c8b662131c..f2188c7b82 100644
--- a/src/backend/commands/event_trigger.c
+++ b/src/backend/commands/event_trigger.c
@@ -1141,128 +1141,26 @@ EventTriggerSupportsObjectType(ObjectType obtype)
case OBJECT_EVENT_TRIGGER:
/* no support for event triggers on event triggers */
return false;
- case OBJECT_ACCESS_METHOD:
- case OBJECT_AGGREGATE:
- case OBJECT_AMOP:
- case OBJECT_AMPROC:
- case OBJECT_ATTRIBUTE:
- case OBJECT_CAST:
- case OBJECT_COLUMN:
- case OBJECT_COLLATION:
- case OBJECT_CONVERSION:
- case OBJECT_DEFACL:
- case OBJECT_DEFAULT:
- case OBJECT_DOMAIN:
- case OBJECT_DOMCONSTRAINT:
- case OBJECT_EXTENSION:
- case OBJECT_FDW:
- case OBJECT_FOREIGN_SERVER:
- case OBJECT_FOREIGN_TABLE:
- case OBJECT_FUNCTION:
- case OBJECT_INDEX:
- case OBJECT_LANGUAGE:
- case OBJECT_LARGEOBJECT:
- case OBJECT_MATVIEW:
- case OBJECT_OPCLASS:
- case OBJECT_OPERATOR:
- case OBJECT_OPFAMILY:
- case OBJECT_POLICY:
- case OBJECT_PROCEDURE:
- case OBJECT_PUBLICATION:
- case OBJECT_PUBLICATION_NAMESPACE:
- case OBJECT_PUBLICATION_REL:
- case OBJECT_ROUTINE:
- case OBJECT_RULE:
- case OBJECT_SCHEMA:
- case OBJECT_SEQUENCE:
- case OBJECT_SUBSCRIPTION:
- case OBJECT_STATISTIC_EXT:
- case OBJECT_TABCONSTRAINT:
- case OBJECT_TABLE:
- case OBJECT_TRANSFORM:
- case OBJECT_TRIGGER:
- case OBJECT_TSCONFIGURATION:
- case OBJECT_TSDICTIONARY:
- case OBJECT_TSPARSER:
- case OBJECT_TSTEMPLATE:
- case OBJECT_TYPE:
- case OBJECT_USER_MAPPING:
- case OBJECT_VIEW:
+ default:
return true;
-
- /*
-* There's intentionally no default: case here; we want
the
-*
Re: Improve readability by using designated initializers when possible
On Fri, Mar 1, 2024 at 12:08 PM 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. > -- 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. v1-0001-Remove-ObjectClass-refactor-dependency.c-event.no-cfbot Description: Binary data
Re: Improve readability by using designated initializers when possible
On Fri, 1 Mar 2024 at 06:08, Michael Paquier 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
From 0d66d374697eff2b276b667ce24cf4599432dbd5 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Tue, 27 Feb 2024 12:26:10 +0100
Subject: [PATCH v9] Simplify pg_enc2gettext_tbl
Use designated initialization of pg_enc2gettext_tbl to simplify the
implementation of raw_pg_bind_textdomain_codeset. Now iteration over the
array is not needed anymore. Instead the desired element can simply be
fetched by its index.
---
src/backend/utils/mb/mbutils.c | 24 --
src/common/encnames.c | 86 +-
src/include/mb/pg_wchar.h | 12 ++---
3 files changed, 57 insertions(+), 65 deletions(-)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index c13f947a827..5818e752280 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -1188,24 +1188,20 @@ static bool
raw_pg_bind_textdomain_codeset(const char *domainname, int encoding)
{
bool elog_ok = (CurrentMemoryContext != NULL);
- int i;
- for (i = 0; pg_enc2gettext_tbl[i].name != NULL; i++)
+ if (!PG_VALID_ENCODING(encoding) || pg_enc2gettext_tbl[encoding] == NULL)
{
- if (pg_enc2gettext_tbl[i].encoding == encoding)
- {
- if (bind_textdomain_codeset(domainname,
- pg_enc2gettext_tbl[i].name) != NULL)
-return true;
+ return false;
+ }
- if (elog_ok)
-elog(LOG, "bind_textdomain_codeset failed");
- else
-write_stderr("bind_textdomain_codeset failed");
+ if (bind_textdomain_codeset(domainname,
+pg_enc2gettext_tbl[encoding]) != NULL)
+ return true;
- break;
- }
- }
+ if (elog_ok)
+ elog(LOG, "bind_textdomain_codeset failed");
+ else
+ write_stderr("bind_textdomain_codeset failed");
return false;
}
diff --git a/src/common/encnames.c b/src/common/encnames.c
index dba6bd2c9ee..910cc2c7e5c 100644
--- a/src/common/encnames.c
+++ b/src/common/encnames.c
@@ -357,50 +357,50 @@ const pg_enc2name pg_enc2name_tbl[] =
* This covers all encodings except MULE_INTERNAL, which is alien to gettext.
* --
*/
-const pg_enc2gettext pg_enc2gettext_tbl[] =
+const char *pg_enc2gettext_tbl[] =
{
- {PG_SQL_ASCII, "US-ASCII"},
- {PG_UTF8, "UTF-8"},
- {PG_LATIN1, "LATIN1"},
- {PG_LATIN2, "LATIN2"},
- {PG_LATIN3, "LATIN3"},
- {PG_LATIN4, "LATIN4"},
- {PG_ISO_8859_5, "ISO-8859-5"},
- {PG_ISO_8859_6, "ISO_8859-6"},
- {PG_ISO_8859_7, "ISO-8859-7"},
- {PG_ISO_8859_8, "ISO-8859-8"},
- {PG_LATIN5, "LATIN5"},
- {PG_LATIN6, "LATIN6"},
- {PG_LATIN7, "LATIN7"},
- {PG_LATIN8, "LATIN8"},
- {PG_LATIN9, "LATIN-9"},
- {PG_LATIN10, "LATIN10"},
- {PG_KOI8R, "KOI8-R"},
- {PG_KOI8U, "KOI8-U"},
- {PG_WIN1250, "CP1250"},
- {PG_WIN1251, "CP1251"},
- {PG_WIN1252, "CP1252"},
- {PG_WIN1253, "CP1253"},
- {PG_WIN1254, "CP1254"},
- {PG_WIN1255, "CP1255"},
- {PG_WIN1256, "CP1256"},
- {PG_WIN1257, "CP1257"},
- {PG_WIN1258, "CP1258"},
- {PG_WIN866, "CP866"},
- {PG_WIN874, "CP874"},
- {PG_EUC_CN, "EUC-CN"},
- {PG_EUC_JP, "EUC-JP"},
- {PG_EUC_KR, "EUC-KR"},
- {PG_EUC_TW, "EUC-TW"},
- {PG_EUC_JIS_2004, "EUC-JP"},
- {PG_SJIS, "SHIFT-JIS"},
- {PG_BIG5, "BIG5"},
- {PG_GBK, "GBK"},
- {PG_UHC, "UHC"},
- {PG_GB18030, "GB18030"},
- {PG_JOHAB, "JOHAB"},
- {PG_SHIFT_JIS_2004, "SHIFT_JISX0213"},
- {0, NULL}
+ [PG_SQL_ASCII] = "US-ASCII",
+ [PG_UTF8] = "UTF-8",
+ [PG_MULE_INTERNAL] = NULL,
+ [PG_LATIN1] = "LATIN1",
+ [PG_LATIN2] = "LATIN2",
+ [PG_LATIN3] = "LATIN3",
+ [PG_LATIN4] = "LATIN4",
+ [PG_ISO_8859_5] = "ISO-8859-5",
+ [PG_ISO_8859_6] = "ISO_8859-6",
+ [PG_ISO_8859_7] = "ISO-8859-7",
+ [PG_ISO_8859_8] = "ISO-8859-8",
+ [PG_LATIN5] = "LATIN5",
+ [PG_LATIN6] = "LATIN6",
+ [PG_LATIN7] = "LATIN7",
+ [PG_LATIN8] = "LATIN8",
+ [PG_LATIN9] = "LATIN-9",
+ [PG_LATIN10] = "LATIN10",
+ [PG_KOI8R] = "KOI8-R",
+ [PG_KOI8U] = "KOI8-U",
+ [PG_WIN1250] = "CP1250",
+ [PG_WIN1251] = "CP1251",
+ [PG_WIN1252] = "CP1252",
+ [PG_WIN1253] = "CP1253",
+ [PG_WIN1254] = "CP1254",
+ [PG_WIN1255] = "CP1255",
+ [PG_WIN1256] = "CP1256",
+ [PG_WIN1257] = "CP1257",
+ [PG_WIN1258] = "CP1258",
+ [PG_WIN866] = "CP866",
+ [PG_WIN874] = "CP874",
+ [PG_EUC_CN] = "EUC-CN",
+ [PG_EUC_JP] = "EUC-JP",
+ [PG_EUC_KR] = "EUC-KR",
+ [PG_EUC_TW] = "EUC-TW",
+ [PG_EUC_JIS_2004] = "EUC-JP",
+ [PG_SJIS] = "SHIFT-JIS",
+ [PG_BIG5] = "BIG5",
+ [PG_GBK] = "GBK",
+ [PG_UHC] = "UHC",
+ [PG_GB18030] = "GB18030",
+ [PG_JOHAB] = "JOHAB",
+ [PG_SHIFT_JIS_2004] = "SHIFT_JISX0213",
};
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index fd91aefbcb7..cc804cc0f5b 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -225,7 +225,9 @@ 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
Re: Improve readability by using designated initializers when possible
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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Fri, 1 Mar 2024 at 05:12, Michael Paquier 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.
From 1a45e03cdaf0c0cf962a33cea8cb25a7c2783d9d Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Tue, 27 Feb 2024 12:26:10 +0100
Subject: [PATCH v8] Simplify pg_enc2gettext_tbl
Use designated initialization of pg_enc2gettext_tbl to simplify the
implementation of raw_pg_bind_textdomain_codeset. Now iteration over the
array is not needed anymore. Instead the desired element can simply be
fetched by its index.
---
src/backend/utils/mb/mbutils.c | 24 --
src/common/encnames.c | 86 +-
src/include/mb/pg_wchar.h | 11 ++---
3 files changed, 56 insertions(+), 65 deletions(-)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index c13f947a827..5818e752280 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -1188,24 +1188,20 @@ static bool
raw_pg_bind_textdomain_codeset(const char *domainname, int encoding)
{
bool elog_ok = (CurrentMemoryContext != NULL);
- int i;
- for (i = 0; pg_enc2gettext_tbl[i].name != NULL; i++)
+ if (!PG_VALID_ENCODING(encoding) || pg_enc2gettext_tbl[encoding] == NULL)
{
- if (pg_enc2gettext_tbl[i].encoding == encoding)
- {
- if (bind_textdomain_codeset(domainname,
- pg_enc2gettext_tbl[i].name) != NULL)
-return true;
+ return false;
+ }
- if (elog_ok)
-elog(LOG, "bind_textdomain_codeset failed");
- else
-write_stderr("bind_textdomain_codeset failed");
+ if (bind_textdomain_codeset(domainname,
+pg_enc2gettext_tbl[encoding]) != NULL)
+ return true;
- break;
- }
- }
+ if (elog_ok)
+ elog(LOG, "bind_textdomain_codeset failed");
+ else
+ write_stderr("bind_textdomain_codeset failed");
return false;
}
diff --git a/src/common/encnames.c b/src/common/encnames.c
index dba6bd2c9ee..910cc2c7e5c 100644
--- a/src/common/encnames.c
+++ b/src/common/encnames.c
@@ -357,50 +357,50 @@ const pg_enc2name pg_enc2name_tbl[] =
* This covers all encodings except MULE_INTERNAL, which is alien to gettext.
* --
*/
-const pg_enc2gettext pg_enc2gettext_tbl[] =
+const char *pg_enc2gettext_tbl[] =
{
- {PG_SQL_ASCII, "US-ASCII"},
- {PG_UTF8, "UTF-8"},
- {PG_LATIN1, "LATIN1"},
- {PG_LATIN2, "LATIN2"},
- {PG_LATIN3, "LATIN3"},
- {PG_LATIN4, "LATIN4"},
- {PG_ISO_8859_5, "ISO-8859-5"},
- {PG_ISO_8859_6, "ISO_8859-6"},
- {PG_ISO_8859_7, "ISO-8859-7"},
- {PG_ISO_8859_8, "ISO-8859-8"},
- {PG_LATIN5, "LATIN5"},
- {PG_LATIN6, "LATIN6"},
- {PG_LATIN7, "LATIN7"},
- {PG_LATIN8, "LATIN8"},
- {PG_LATIN9, "LATIN-9"},
- {PG_LATIN10, "LATIN10"},
- {PG_KOI8R, "KOI8-R"},
- {PG_KOI8U, "KOI8-U"},
- {PG_WIN1250, "CP1250"},
- {PG_WIN1251, "CP1251"},
- {PG_WIN1252, "CP1252"},
- {PG_WIN1253, "CP1253"},
- {PG_WIN1254, "CP1254"},
- {PG_WIN1255, "CP1255"},
- {PG_WIN1256, "CP1256"},
- {PG_WIN1257, "CP1257"},
- {PG_WIN1258, "CP1258"},
- {PG_WIN866, "CP866"},
- {PG_WIN874, "CP874"},
- {PG_EUC_CN, "EUC-CN"},
- {PG_EUC_JP, "EUC-JP"},
- {PG_EUC_KR, "EUC-KR"},
- {PG_EUC_TW, "EUC-TW"},
- {PG_EUC_JIS_2004, "EUC-JP"},
- {PG_SJIS, "SHIFT-JIS"},
- {PG_BIG5, "BIG5"},
- {PG_GBK, "GBK"},
- {PG_UHC, "UHC"},
- {PG_GB18030, "GB18030"},
- {PG_JOHAB, "JOHAB"},
- {PG_SHIFT_JIS_2004, "SHIFT_JISX0213"},
- {0, NULL}
+ [PG_SQL_ASCII] = "US-ASCII",
+ [PG_UTF8] = "UTF-8",
+ [PG_MULE_INTERNAL] = NULL,
+ [PG_LATIN1] = "LATIN1",
+ [PG_LATIN2] = "LATIN2",
+ [PG_LATIN3] = "LATIN3",
+ [PG_LATIN4] = "LATIN4",
+ [PG_ISO_8859_5] = "ISO-8859-5",
+ [PG_ISO_8859_6] = "ISO_8859-6",
+ [PG_ISO_8859_7] = "ISO-8859-7",
+ [PG_ISO_8859_8] = "ISO-8859-8",
+ [PG_LATIN5] = "LATIN5",
+ [PG_LATIN6] = "LATIN6",
+ [PG_LATIN7] = "LATIN7",
+ [PG_LATIN8] = "LATIN8",
+ [PG_LATIN9] = "LATIN-9",
+ [PG_LATIN10] = "LATIN10",
+ [PG_KOI8R] = "KOI8-R",
+ [PG_KOI8U] = "KOI8-U",
+ [PG_WIN1250] = "CP1250",
+ [PG_WIN1251] = "CP1251",
+ [PG_WIN1252] = "CP1252",
+ [PG_WIN1253] = "CP1253",
+ [PG_WIN1254] = "CP1254",
+ [PG_WIN1255] = "CP1255",
+ [PG_WIN1256] = "CP1256",
+ [PG_WIN1257] = "CP1257",
+ [PG_WIN1258] = "CP1258",
+ [PG_WIN866] = "CP866",
+ [PG_WIN874] = "CP874",
+ [PG_EUC_CN] = "EUC-CN",
+ [PG_EUC_JP] = "EUC-JP",
+ [PG_EUC_KR] = "EUC-KR",
+ [PG_EUC_TW] = "EUC-TW",
+ [PG_EUC_JIS_2004] = "EUC-JP",
+ [PG_SJIS] = "SHIFT-JIS",
+ [PG_BIG5] = "BIG5",
+ [PG_GBK] = "GBK",
+ [PG_UHC] = "UHC",
+ [PG_GB18030] = "GB18030",
+ [PG_JOHAB] = "JOHAB",
+ [PG_SHIFT_JIS_2004] = "SHIFT_JISX0213",
};
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
Re: Improve readability by using designated initializers when possible
On Thu, Feb 29, 2024 at 04:01:47AM +0100, Jelte Fennema-Nio wrote:
> On Thu, 29 Feb 2024 at 01:57, Michael Paquier 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
signature.asc
Description: PGP signature
Re: Improve readability by using designated initializers when possible
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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
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.
From c3acb638301b7a00d18baaf464c29f742734629a Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Thu, 29 Feb 2024 12:06:15 +0100
Subject: [PATCH] Remove ObjectClass
---
src/backend/catalog/dependency.c| 223 +++-
src/backend/catalog/objectaddress.c | 311
src/backend/commands/alter.c| 73 ++-
src/backend/commands/tablecmds.c| 60 ++
src/include/catalog/dependency.h| 51 -
5 files changed, 188 insertions(+), 530 deletions(-)
diff --git a/src/backend/catalog/dependency.c b/src/backend/catalog/dependency.c
index 192dedb3cb..d6a5d1d185 100644
--- a/src/backend/catalog/dependency.c
+++ b/src/backend/catalog/dependency.c
@@ -1351,9 +1351,9 @@ deleteOneObject(const ObjectAddress *object, Relation
*depRel, int flags)
static void
doDeletion(const ObjectAddress *object, int flags)
{
- switch (getObjectClass(object))
+ switch (object->classId)
{
- case OCLASS_CLASS:
+ case RelationRelationId:
{
charrelKind =
get_rel_relkind(object->objectId);
@@ -1384,104 +1384,80 @@ doDeletion(const ObjectAddress *object, int flags)
break;
}
- case OCLASS_PROC:
+ case ProcedureRelationId:
RemoveFunctionById(object->objectId);
break;
- case OCLASS_TYPE:
+ case TypeRelationId:
RemoveTypeById(object->objectId);
break;
- case OCLASS_CONSTRAINT:
+ case ConstraintRelationId:
RemoveConstraintById(object->objectId);
break;
- case OCLASS_DEFAULT:
+ case AttrDefaultRelationId:
RemoveAttrDefaultById(object->objectId);
break;
- case OCLASS_LARGEOBJECT:
+ case LargeObjectRelationId:
LargeObjectDrop(object->objectId);
break;
- case OCLASS_OPERATOR:
+ case OperatorRelationId:
RemoveOperatorById(object->objectId);
break;
- case OCLASS_REWRITE:
+ case RewriteRelationId:
RemoveRewriteRuleById(object->objectId);
break;
- case OCLASS_TRIGGER:
+ case TriggerRelationId:
RemoveTriggerById(object->objectId);
break;
- case OCLASS_STATISTIC_EXT:
+ case StatisticExtRelationId:
RemoveStatisticsById(object->objectId);
break;
- case OCLASS_TSCONFIG:
+ case TSConfigRelationId:
RemoveTSConfigurationById(object->objectId);
break;
- case OCLASS_EXTENSION:
+ case ExtensionRelationId:
Re: Improve readability by using designated initializers when possible
On Thu, 29 Feb 2024 at 01:57, Michael Paquier 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.
From a51592bda622746b9f015d7993ca19254bedbc0e Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Tue, 27 Feb 2024 12:26:10 +0100
Subject: [PATCH v7] Simplify pg_enc2gettext_tbl
Use designated initialization of pg_enc2gettext_tbl to simplify the
implementation of raw_pg_bind_textdomain_codeset. Now iteration over the
array is not needed anymore. Instead the desired element can simply be
fetched by its index.
---
src/backend/utils/mb/mbutils.c | 24 --
src/common/encnames.c | 85 +-
src/include/mb/pg_wchar.h | 11 ++---
3 files changed, 55 insertions(+), 65 deletions(-)
diff --git a/src/backend/utils/mb/mbutils.c b/src/backend/utils/mb/mbutils.c
index c13f947a827..5818e752280 100644
--- a/src/backend/utils/mb/mbutils.c
+++ b/src/backend/utils/mb/mbutils.c
@@ -1188,24 +1188,20 @@ static bool
raw_pg_bind_textdomain_codeset(const char *domainname, int encoding)
{
bool elog_ok = (CurrentMemoryContext != NULL);
- int i;
- for (i = 0; pg_enc2gettext_tbl[i].name != NULL; i++)
+ if (!PG_VALID_ENCODING(encoding) || pg_enc2gettext_tbl[encoding] == NULL)
{
- if (pg_enc2gettext_tbl[i].encoding == encoding)
- {
- if (bind_textdomain_codeset(domainname,
- pg_enc2gettext_tbl[i].name) != NULL)
-return true;
+ return false;
+ }
- if (elog_ok)
-elog(LOG, "bind_textdomain_codeset failed");
- else
-write_stderr("bind_textdomain_codeset failed");
+ if (bind_textdomain_codeset(domainname,
+pg_enc2gettext_tbl[encoding]) != NULL)
+ return true;
- break;
- }
- }
+ if (elog_ok)
+ elog(LOG, "bind_textdomain_codeset failed");
+ else
+ write_stderr("bind_textdomain_codeset failed");
return false;
}
diff --git a/src/common/encnames.c b/src/common/encnames.c
index dba6bd2c9ee..d94c740a28b 100644
--- a/src/common/encnames.c
+++ b/src/common/encnames.c
@@ -357,50 +357,49 @@ const pg_enc2name pg_enc2name_tbl[] =
* This covers all encodings except MULE_INTERNAL, which is alien to gettext.
* --
*/
-const pg_enc2gettext pg_enc2gettext_tbl[] =
+const char *pg_enc2gettext_tbl[] =
{
- {PG_SQL_ASCII, "US-ASCII"},
- {PG_UTF8, "UTF-8"},
- {PG_LATIN1, "LATIN1"},
- {PG_LATIN2, "LATIN2"},
- {PG_LATIN3, "LATIN3"},
- {PG_LATIN4, "LATIN4"},
- {PG_ISO_8859_5, "ISO-8859-5"},
- {PG_ISO_8859_6, "ISO_8859-6"},
- {PG_ISO_8859_7, "ISO-8859-7"},
- {PG_ISO_8859_8, "ISO-8859-8"},
- {PG_LATIN5, "LATIN5"},
- {PG_LATIN6, "LATIN6"},
- {PG_LATIN7, "LATIN7"},
- {PG_LATIN8, "LATIN8"},
- {PG_LATIN9, "LATIN-9"},
- {PG_LATIN10, "LATIN10"},
- {PG_KOI8R, "KOI8-R"},
- {PG_KOI8U, "KOI8-U"},
- {PG_WIN1250, "CP1250"},
- {PG_WIN1251, "CP1251"},
- {PG_WIN1252, "CP1252"},
- {PG_WIN1253, "CP1253"},
- {PG_WIN1254, "CP1254"},
- {PG_WIN1255, "CP1255"},
- {PG_WIN1256, "CP1256"},
- {PG_WIN1257, "CP1257"},
- {PG_WIN1258, "CP1258"},
- {PG_WIN866, "CP866"},
- {PG_WIN874, "CP874"},
- {PG_EUC_CN, "EUC-CN"},
- {PG_EUC_JP, "EUC-JP"},
- {PG_EUC_KR, "EUC-KR"},
- {PG_EUC_TW, "EUC-TW"},
- {PG_EUC_JIS_2004, "EUC-JP"},
- {PG_SJIS, "SHIFT-JIS"},
- {PG_BIG5, "BIG5"},
- {PG_GBK, "GBK"},
- {PG_UHC, "UHC"},
- {PG_GB18030, "GB18030"},
- {PG_JOHAB, "JOHAB"},
- {PG_SHIFT_JIS_2004, "SHIFT_JISX0213"},
- {0, NULL}
+ [PG_SQL_ASCII] = "US-ASCII",
+ [PG_UTF8] = "UTF-8",
+ [PG_LATIN1] = "LATIN1",
+ [PG_LATIN2] = "LATIN2",
+ [PG_LATIN3] = "LATIN3",
+ [PG_LATIN4] = "LATIN4",
+ [PG_ISO_8859_5] = "ISO-8859-5",
+ [PG_ISO_8859_6] = "ISO_8859-6",
+ [PG_ISO_8859_7] = "ISO-8859-7",
+ [PG_ISO_8859_8] = "ISO-8859-8",
+ [PG_LATIN5] = "LATIN5",
+ [PG_LATIN6] = "LATIN6",
+ [PG_LATIN7] = "LATIN7",
+ [PG_LATIN8] = "LATIN8",
+ [PG_LATIN9] = "LATIN-9",
+ [PG_LATIN10] = "LATIN10",
+ [PG_KOI8R] = "KOI8-R",
+ [PG_KOI8U] = "KOI8-U",
+ [PG_WIN1250] = "CP1250",
+ [PG_WIN1251] = "CP1251",
+ [PG_WIN1252] = "CP1252",
+ [PG_WIN1253] = "CP1253",
+ [PG_WIN1254] = "CP1254",
+ [PG_WIN1255] = "CP1255",
+ [PG_WIN1256] = "CP1256",
+ [PG_WIN1257] = "CP1257",
+ [PG_WIN1258] = "CP1258",
+ [PG_WIN866] = "CP866",
+ [PG_WIN874] = "CP874",
+ [PG_EUC_CN] = "EUC-CN",
+ [PG_EUC_JP] = "EUC-JP",
+ [PG_EUC_KR] = "EUC-KR",
+ [PG_EUC_TW] = "EUC-TW",
+ [PG_EUC_JIS_2004] = "EUC-JP",
+ [PG_SJIS] = "SHIFT-JIS",
+ [PG_BIG5] = "BIG5",
+ [PG_GBK] = "GBK",
+ [PG_UHC] = "UHC",
+ [PG_GB18030] = "GB18030",
+ [PG_JOHAB] = "JOHAB",
+ [PG_SHIFT_JIS_2004] = "SHIFT_JISX0213",
};
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index
Re: Improve readability by using designated initializers when possible
On Wed, Feb 28, 2024 at 05:37:22AM +0100, Jelte Fennema-Nio wrote:
> On Wed, 28 Feb 2024 at 04:59, Michael Paquier 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
signature.asc
Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Wed, 28 Feb 2024 at 04:59, Michael Paquier 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).
Re: Improve readability by using designated initializers when possible
On Wed, Feb 28, 2024 at 09:41:42AM +0800, Japin Li wrote:
> On Wed, 28 Feb 2024 at 00:06, Jelte Fennema-Nio 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
From e42d47cf5854932d0bfcf713ec47a9d5ca060c4f Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 21 Feb 2024 15:34:38 +0100
Subject: [PATCH v6 2/3] Use designated initializer syntax to improve
readability
Use C99 designated initializer syntax for array elements of various
arrays. This is less verbose, more readable and less prone to typos or
ordering mistakes.
Akin to 74a730631065 and cc150596341e.
---
src/include/mb/pg_wchar.h | 6 +-
src/common/encnames.c | 155 +++---
src/common/wchar.c| 85 +++--
3 files changed, 120 insertions(+), 126 deletions(-)
diff --git a/src/include/mb/pg_wchar.h b/src/include/mb/pg_wchar.h
index 1d521bea24..9248daab3a 100644
--- a/src/include/mb/pg_wchar.h
+++ b/src/include/mb/pg_wchar.h
@@ -224,11 +224,7 @@ typedef unsigned int pg_wchar;
/*
* PostgreSQL encoding identifiers
*
- * 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.
*
* PG_SQL_ASCII is default encoding and must be = 0.
diff --git a/src/common/encnames.c b/src/common/encnames.c
index 710b747f6b..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
@@ -308,48 +307,48 @@ static const pg_encname pg_encname_tbl[] =
const pg_enc2name pg_enc2name_tbl[] =
{
- DEF_ENC2NAME(SQL_ASCII, 0),
- DEF_ENC2NAME(EUC_JP, 20932),
- DEF_ENC2NAME(EUC_CN, 20936),
- DEF_ENC2NAME(EUC_KR, 51949),
- DEF_ENC2NAME(EUC_TW, 0),
- DEF_ENC2NAME(EUC_JIS_2004, 20932),
- DEF_ENC2NAME(UTF8, 65001),
- DEF_ENC2NAME(MULE_INTERNAL, 0),
- DEF_ENC2NAME(LATIN1, 28591),
- DEF_ENC2NAME(LATIN2, 28592),
- DEF_ENC2NAME(LATIN3, 28593),
- DEF_ENC2NAME(LATIN4, 28594),
- DEF_ENC2NAME(LATIN5, 28599),
- DEF_ENC2NAME(LATIN6, 0),
- DEF_ENC2NAME(LATIN7, 0),
- DEF_ENC2NAME(LATIN8, 0),
- DEF_ENC2NAME(LATIN9, 28605),
- DEF_ENC2NAME(LATIN10, 0),
- DEF_ENC2NAME(WIN1256, 1256),
- DEF_ENC2NAME(WIN1258, 1258),
- DEF_ENC2NAME(WIN866, 866),
- DEF_ENC2NAME(WIN874, 874),
- DEF_ENC2NAME(KOI8R, 20866),
- DEF_ENC2NAME(WIN1251, 1251),
- DEF_ENC2NAME(WIN1252, 1252),
- DEF_ENC2NAME(ISO_8859_5, 28595),
- DEF_ENC2NAME(ISO_8859_6, 28596),
- DEF_ENC2NAME(ISO_8859_7, 28597),
- DEF_ENC2NAME(ISO_8859_8, 28598),
- DEF_ENC2NAME(WIN1250, 1250),
- DEF_ENC2NAME(WIN1253, 1253),
- DEF_ENC2NAME(WIN1254, 1254),
- DEF_ENC2NAME(WIN1255, 1255),
- DEF_ENC2NAME(WIN1257, 1257),
- DEF_ENC2NAME(KOI8U, 21866),
- DEF_ENC2NAME(SJIS, 932),
- DEF_ENC2NAME(BIG5, 950),
- DEF_ENC2NAME(GBK, 936),
- DEF_ENC2NAME(UHC, 949),
- DEF_ENC2NAME(GB18030, 54936),
- DEF_ENC2NAME(JOHAB, 0),
- DEF_ENC2NAME(SHIFT_JIS_2004, 932)
+ [PG_SQL_ASCII] = DEF_ENC2NAME(SQL_ASCII, 0),
+ [PG_EUC_JP] = DEF_ENC2NAME(EUC_JP, 20932),
+ [PG_EUC_CN] = DEF_ENC2NAME(EUC_CN, 20936),
+ [PG_EUC_KR] = DEF_ENC2NAME(EUC_KR, 51949),
+ [PG_EUC_TW] = DEF_ENC2NAME(EUC_TW, 0),
+ [PG_EUC_JIS_2004] = DEF_ENC2NAME(EUC_JIS_2004, 20932),
+ [PG_UTF8] = DEF_ENC2NAME(UTF8, 65001),
+ [PG_MULE_INTERNAL] = DEF_ENC2NAME(MULE_INTERNAL, 0),
+ [PG_LATIN1] = DEF_ENC2NAME(LATIN1, 28591),
+ [PG_LATIN2] = DEF_ENC2NAME(LATIN2, 28592),
+ [PG_LATIN3] = DEF_ENC2NAME(LATIN3, 28593),
+ [PG_LATIN4] = DEF_ENC2NAME(LATIN4, 28594),
+ [PG_LATIN5] = DEF_ENC2NAME(LATIN5, 28599),
+ [PG_LATIN6] = DEF_ENC2NAME(LATIN6, 0),
+ [PG_LATIN7] = DEF_ENC2NAME(LATIN7, 0),
+ [PG_LATIN8] = DEF_ENC2NAME(LATIN8, 0),
+ [PG_LATIN9] = DEF_ENC2NAME(LATIN9, 28605),
+ [PG_LATIN10] = DEF_ENC2NAME(LATIN10, 0),
+ [PG_WIN1256] = DEF_ENC2NAME(WIN1256, 1256),
+ [PG_WIN1258] = DEF_ENC2NAME(WIN1258, 1258),
+ [PG_WIN866] = DEF_ENC2NAME(WIN866, 866),
+ [PG_WIN874] = DEF_ENC2NAME(WIN874, 874),
+ [PG_KOI8R] = DEF_ENC2NAME(KOI8R, 20866),
+ [PG_WIN1251] = DEF_ENC2NAME(WIN1251, 1251),
+ [PG_WIN1252] = DEF_ENC2NAME(WIN1252, 1252),
+ [PG_ISO_8859_5] = DEF_ENC2NAME(ISO_8859_5, 28595),
+ [PG_ISO_8859_6] = DEF_ENC2NAME(ISO_8859_6, 28596),
+ [PG_ISO_8859_7] = DEF_ENC2NAME(ISO_8859_7, 28597),
+ [PG_ISO_8859_8] = DEF_ENC2NAME(ISO_8859_8, 28598),
+ [PG_WIN1250] =
Re: Improve readability by using designated initializers when possible
On Wed, 28 Feb 2024 at 00:06, Jelte Fennema-Nio wrote: > On Tue, 27 Feb 2024 at 16:04, Japin Li 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
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 16:04, Japin Li 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).
From 945fae648d0a396e9f99413c8e31542ced9b17ba Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 21 Feb 2024 15:34:38 +0100
Subject: [PATCH v5 2/4] Use designated initializer syntax to improve
readability
Use C99 designated initializer syntax for array elements of various
arrays. This is less verbose, more readable and less prone to typos or
ordering mistakes.
Akin to 74a730631065 and cc150596341e.
---
src/backend/parser/parser.c | 12 +-
src/backend/utils/misc/guc_tables.c | 187 +++-
src/bin/pg_dump/pg_dump_sort.c | 94 +++---
src/common/encnames.c | 154 +++
src/common/relpath.c| 8 +-
src/common/wchar.c | 85 +++--
src/include/mb/pg_wchar.h | 6 +-
7 files changed, 248 insertions(+), 298 deletions(-)
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 9ec628ecbdf..3a1fa91c1b6 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -56,12 +56,12 @@ raw_parser(const char *str, RawParseMode mode)
{
/* this array is indexed by RawParseMode enum */
static const int mode_token[] = {
- 0, /* RAW_PARSE_DEFAULT */
- MODE_TYPE_NAME, /* RAW_PARSE_TYPE_NAME */
- MODE_PLPGSQL_EXPR, /* RAW_PARSE_PLPGSQL_EXPR */
- MODE_PLPGSQL_ASSIGN1, /* RAW_PARSE_PLPGSQL_ASSIGN1 */
- MODE_PLPGSQL_ASSIGN2, /* RAW_PARSE_PLPGSQL_ASSIGN2 */
- MODE_PLPGSQL_ASSIGN3 /* RAW_PARSE_PLPGSQL_ASSIGN3 */
+ [RAW_PARSE_DEFAULT] = 0,
+ [RAW_PARSE_TYPE_NAME] = MODE_TYPE_NAME,
+ [RAW_PARSE_PLPGSQL_EXPR] = MODE_PLPGSQL_EXPR,
+ [RAW_PARSE_PLPGSQL_ASSIGN1] = MODE_PLPGSQL_ASSIGN1,
+ [RAW_PARSE_PLPGSQL_ASSIGN2] = MODE_PLPGSQL_ASSIGN2,
+ [RAW_PARSE_PLPGSQL_ASSIGN3] = MODE_PLPGSQL_ASSIGN3,
};
yyextra.have_lookahead = true;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 527a2b27340..a63ea042edf 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -627,13 +627,13 @@ bool in_hot_standby_guc;
*/
const char *const GucContext_Names[] =
{
- /* PGC_INTERNAL */ "internal",
- /* PGC_POSTMASTER */ "postmaster",
- /* PGC_SIGHUP */ "sighup",
- /* PGC_SU_BACKEND */ "superuser-backend",
- /* PGC_BACKEND */ "backend",
- /* PGC_SUSET */ "superuser",
- /* PGC_USERSET */ "user"
+ [PGC_INTERNAL] = "internal",
+ [PGC_POSTMASTER] = "postmaster",
+ [PGC_SIGHUP] = "sighup",
+ [PGC_SU_BACKEND] = "superuser-backend",
+ [PGC_BACKEND] = "backend",
+ [PGC_SUSET] = "superuser",
+ [PGC_USERSET] = "user",
};
StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
@@ -646,20 +646,20 @@ StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
*/
const char *const GucSource_Names[] =
{
- /* PGC_S_DEFAULT */ "default",
- /* PGC_S_DYNAMIC_DEFAULT */ "default",
- /* PGC_S_ENV_VAR */ "environment variable",
- /* PGC_S_FILE */ "configuration file",
- /* PGC_S_ARGV */ "command line",
- /* PGC_S_GLOBAL */ "global",
- /* PGC_S_DATABASE */ "database",
- /* PGC_S_USER */ "user",
- /* PGC_S_DATABASE_USER */ "database user",
- /* PGC_S_CLIENT */ "client",
- /* PGC_S_OVERRIDE */ "override",
- /* PGC_S_INTERACTIVE */ "interactive",
- /* PGC_S_TEST */ "test",
- /* PGC_S_SESSION */ "session"
+ [PGC_S_DEFAULT] = "default",
+ [PGC_S_DYNAMIC_DEFAULT] = "default",
+ [PGC_S_ENV_VAR] = "environment variable",
+ [PGC_S_FILE] = "configuration file",
+ [PGC_S_ARGV] = "command line",
+ [PGC_S_GLOBAL] = "global",
+ [PGC_S_DATABASE] = "database",
+ [PGC_S_USER] = "user",
+ [PGC_S_DATABASE_USER] = "database user",
+ [PGC_S_CLIENT] = "client",
+ [PGC_S_OVERRIDE] = "override",
+ [PGC_S_INTERACTIVE] = "interactive",
+ [PGC_S_TEST] = "test",
+ [PGC_S_SESSION] = "session",
};
StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
@@ -670,96 +670,51 @@ StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
*/
const char *const config_group_names[] =
{
- /* UNGROUPED */
- gettext_noop("Ungrouped"),
- /* FILE_LOCATIONS */
- gettext_noop("File Locations"),
- /* CONN_AUTH_SETTINGS */
- gettext_noop("Connections and Authentication / Connection Settings"),
- /* CONN_AUTH_TCP */
- gettext_noop("Connections and Authentication / TCP Settings"),
- /* CONN_AUTH_AUTH */
- gettext_noop("Connections and Authentication / Authentication"),
- /* CONN_AUTH_SSL */
- gettext_noop("Connections and Authentication / SSL"),
- /* RESOURCES_MEM */
- gettext_noop("Resource Usage /
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 19:55, Jelte Fennema-Nio wrote:
> On Tue, 27 Feb 2024 at 12:52, Jelte Fennema-Nio 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");
/*
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 08:57, Alvaro Herrera 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
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 12:52, Jelte Fennema-Nio 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).
From 945fae648d0a396e9f99413c8e31542ced9b17ba Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 21 Feb 2024 15:34:38 +0100
Subject: [PATCH v4 2/3] Use designated initializer syntax to improve
readability
Use C99 designated initializer syntax for array elements of various
arrays. This is less verbose, more readable and less prone to typos or
ordering mistakes.
Akin to 74a730631065 and cc150596341e.
---
src/backend/parser/parser.c | 12 +-
src/backend/utils/misc/guc_tables.c | 187 +++-
src/bin/pg_dump/pg_dump_sort.c | 94 +++---
src/common/encnames.c | 154 +++
src/common/relpath.c| 8 +-
src/common/wchar.c | 85 +++--
src/include/mb/pg_wchar.h | 6 +-
7 files changed, 248 insertions(+), 298 deletions(-)
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 9ec628ecbdf..3a1fa91c1b6 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -56,12 +56,12 @@ raw_parser(const char *str, RawParseMode mode)
{
/* this array is indexed by RawParseMode enum */
static const int mode_token[] = {
- 0, /* RAW_PARSE_DEFAULT */
- MODE_TYPE_NAME, /* RAW_PARSE_TYPE_NAME */
- MODE_PLPGSQL_EXPR, /* RAW_PARSE_PLPGSQL_EXPR */
- MODE_PLPGSQL_ASSIGN1, /* RAW_PARSE_PLPGSQL_ASSIGN1 */
- MODE_PLPGSQL_ASSIGN2, /* RAW_PARSE_PLPGSQL_ASSIGN2 */
- MODE_PLPGSQL_ASSIGN3 /* RAW_PARSE_PLPGSQL_ASSIGN3 */
+ [RAW_PARSE_DEFAULT] = 0,
+ [RAW_PARSE_TYPE_NAME] = MODE_TYPE_NAME,
+ [RAW_PARSE_PLPGSQL_EXPR] = MODE_PLPGSQL_EXPR,
+ [RAW_PARSE_PLPGSQL_ASSIGN1] = MODE_PLPGSQL_ASSIGN1,
+ [RAW_PARSE_PLPGSQL_ASSIGN2] = MODE_PLPGSQL_ASSIGN2,
+ [RAW_PARSE_PLPGSQL_ASSIGN3] = MODE_PLPGSQL_ASSIGN3,
};
yyextra.have_lookahead = true;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 527a2b27340..a63ea042edf 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -627,13 +627,13 @@ bool in_hot_standby_guc;
*/
const char *const GucContext_Names[] =
{
- /* PGC_INTERNAL */ "internal",
- /* PGC_POSTMASTER */ "postmaster",
- /* PGC_SIGHUP */ "sighup",
- /* PGC_SU_BACKEND */ "superuser-backend",
- /* PGC_BACKEND */ "backend",
- /* PGC_SUSET */ "superuser",
- /* PGC_USERSET */ "user"
+ [PGC_INTERNAL] = "internal",
+ [PGC_POSTMASTER] = "postmaster",
+ [PGC_SIGHUP] = "sighup",
+ [PGC_SU_BACKEND] = "superuser-backend",
+ [PGC_BACKEND] = "backend",
+ [PGC_SUSET] = "superuser",
+ [PGC_USERSET] = "user",
};
StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
@@ -646,20 +646,20 @@ StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
*/
const char *const GucSource_Names[] =
{
- /* PGC_S_DEFAULT */ "default",
- /* PGC_S_DYNAMIC_DEFAULT */ "default",
- /* PGC_S_ENV_VAR */ "environment variable",
- /* PGC_S_FILE */ "configuration file",
- /* PGC_S_ARGV */ "command line",
- /* PGC_S_GLOBAL */ "global",
- /* PGC_S_DATABASE */ "database",
- /* PGC_S_USER */ "user",
- /* PGC_S_DATABASE_USER */ "database user",
- /* PGC_S_CLIENT */ "client",
- /* PGC_S_OVERRIDE */ "override",
- /* PGC_S_INTERACTIVE */ "interactive",
- /* PGC_S_TEST */ "test",
- /* PGC_S_SESSION */ "session"
+ [PGC_S_DEFAULT] = "default",
+ [PGC_S_DYNAMIC_DEFAULT] = "default",
+ [PGC_S_ENV_VAR] = "environment variable",
+ [PGC_S_FILE] = "configuration file",
+ [PGC_S_ARGV] = "command line",
+ [PGC_S_GLOBAL] = "global",
+ [PGC_S_DATABASE] = "database",
+ [PGC_S_USER] = "user",
+ [PGC_S_DATABASE_USER] = "database user",
+ [PGC_S_CLIENT] = "client",
+ [PGC_S_OVERRIDE] = "override",
+ [PGC_S_INTERACTIVE] = "interactive",
+ [PGC_S_TEST] = "test",
+ [PGC_S_SESSION] = "session",
};
StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
@@ -670,96 +670,51 @@ StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
*/
const char *const config_group_names[] =
{
- /* UNGROUPED */
- gettext_noop("Ungrouped"),
- /* FILE_LOCATIONS */
- gettext_noop("File Locations"),
- /* CONN_AUTH_SETTINGS */
- gettext_noop("Connections and Authentication / Connection Settings"),
- /* CONN_AUTH_TCP */
- gettext_noop("Connections and Authentication / TCP Settings"),
- /* CONN_AUTH_AUTH */
- gettext_noop("Connections and Authentication / Authentication"),
- /* CONN_AUTH_SSL */
- gettext_noop("Connections and Authentication / SSL"),
- /* RESOURCES_MEM */
- gettext_noop("Resource Usage / Memory"),
- /* RESOURCES_DISK */
- gettext_noop("Resource Usage / Disk"),
- /* RESOURCES_KERNEL */
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 07:25, Michael Paquier 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.
From 4bdf8991d948a251cdade89dbacf121c64f420c7 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio
Date: Wed, 21 Feb 2024 15:34:38 +0100
Subject: [PATCH v3 2/3] Use designated initializer syntax to improve
readability
Use C99 designated initializer syntax for array elements of various
arrays. This is less verbose, more readable and less prone to typos or
ordering mistakes.
Akin to 74a730631065 and cc150596341e.
---
src/backend/parser/parser.c | 12 +-
src/backend/utils/misc/guc_tables.c | 187 +++-
src/bin/pg_dump/pg_dump_sort.c | 94 +++---
src/common/encnames.c | 154 +++
src/common/relpath.c| 8 +-
src/common/wchar.c | 85 +++--
src/include/mb/pg_wchar.h | 6 +-
7 files changed, 248 insertions(+), 298 deletions(-)
diff --git a/src/backend/parser/parser.c b/src/backend/parser/parser.c
index 9ec628ecbdf..3a1fa91c1b6 100644
--- a/src/backend/parser/parser.c
+++ b/src/backend/parser/parser.c
@@ -56,12 +56,12 @@ raw_parser(const char *str, RawParseMode mode)
{
/* this array is indexed by RawParseMode enum */
static const int mode_token[] = {
- 0, /* RAW_PARSE_DEFAULT */
- MODE_TYPE_NAME, /* RAW_PARSE_TYPE_NAME */
- MODE_PLPGSQL_EXPR, /* RAW_PARSE_PLPGSQL_EXPR */
- MODE_PLPGSQL_ASSIGN1, /* RAW_PARSE_PLPGSQL_ASSIGN1 */
- MODE_PLPGSQL_ASSIGN2, /* RAW_PARSE_PLPGSQL_ASSIGN2 */
- MODE_PLPGSQL_ASSIGN3 /* RAW_PARSE_PLPGSQL_ASSIGN3 */
+ [RAW_PARSE_DEFAULT] = 0,
+ [RAW_PARSE_TYPE_NAME] = MODE_TYPE_NAME,
+ [RAW_PARSE_PLPGSQL_EXPR] = MODE_PLPGSQL_EXPR,
+ [RAW_PARSE_PLPGSQL_ASSIGN1] = MODE_PLPGSQL_ASSIGN1,
+ [RAW_PARSE_PLPGSQL_ASSIGN2] = MODE_PLPGSQL_ASSIGN2,
+ [RAW_PARSE_PLPGSQL_ASSIGN3] = MODE_PLPGSQL_ASSIGN3,
};
yyextra.have_lookahead = true;
diff --git a/src/backend/utils/misc/guc_tables.c b/src/backend/utils/misc/guc_tables.c
index 527a2b27340..59904fd007f 100644
--- a/src/backend/utils/misc/guc_tables.c
+++ b/src/backend/utils/misc/guc_tables.c
@@ -627,13 +627,13 @@ bool in_hot_standby_guc;
*/
const char *const GucContext_Names[] =
{
- /* PGC_INTERNAL */ "internal",
- /* PGC_POSTMASTER */ "postmaster",
- /* PGC_SIGHUP */ "sighup",
- /* PGC_SU_BACKEND */ "superuser-backend",
- /* PGC_BACKEND */ "backend",
- /* PGC_SUSET */ "superuser",
- /* PGC_USERSET */ "user"
+ [PGC_INTERNAL] = "internal",
+ [PGC_POSTMASTER] = "postmaster",
+ [PGC_SIGHUP] = "sighup",
+ [PGC_SU_BACKEND] = "superuser-backend",
+ [PGC_BACKEND] = "backend",
+ [PGC_SUSET] = "superuser",
+ [PGC_USERSET] = "user"
};
StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
@@ -646,20 +646,20 @@ StaticAssertDecl(lengthof(GucContext_Names) == (PGC_USERSET + 1),
*/
const char *const GucSource_Names[] =
{
- /* PGC_S_DEFAULT */ "default",
- /* PGC_S_DYNAMIC_DEFAULT */ "default",
- /* PGC_S_ENV_VAR */ "environment variable",
- /* PGC_S_FILE */ "configuration file",
- /* PGC_S_ARGV */ "command line",
- /* PGC_S_GLOBAL */ "global",
- /* PGC_S_DATABASE */ "database",
- /* PGC_S_USER */ "user",
- /* PGC_S_DATABASE_USER */ "database user",
- /* PGC_S_CLIENT */ "client",
- /* PGC_S_OVERRIDE */ "override",
- /* PGC_S_INTERACTIVE */ "interactive",
- /* PGC_S_TEST */ "test",
- /* PGC_S_SESSION */ "session"
+ [PGC_S_DEFAULT] = "default",
+ [PGC_S_DYNAMIC_DEFAULT] = "default",
+ [PGC_S_ENV_VAR] = "environment variable",
+ [PGC_S_FILE] = "configuration file",
+ [PGC_S_ARGV] = "command line",
+ [PGC_S_GLOBAL] = "global",
+ [PGC_S_DATABASE] = "database",
+ [PGC_S_USER] = "user",
+ [PGC_S_DATABASE_USER] = "database user",
+ [PGC_S_CLIENT] = "client",
+ [PGC_S_OVERRIDE] = "override",
+ [PGC_S_INTERACTIVE] = "interactive",
+ [PGC_S_TEST] = "test",
+ [PGC_S_SESSION] = "session"
};
StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
@@ -670,96 +670,51 @@ StaticAssertDecl(lengthof(GucSource_Names) == (PGC_S_SESSION + 1),
*/
const char *const config_group_names[] =
{
- /* UNGROUPED */
- gettext_noop("Ungrouped"),
- /* FILE_LOCATIONS */
- gettext_noop("File Locations"),
- /* CONN_AUTH_SETTINGS */
- gettext_noop("Connections and Authentication / Connection Settings"),
- /* CONN_AUTH_TCP */
- gettext_noop("Connections and Authentication / TCP Settings"),
- /* CONN_AUTH_AUTH */
- gettext_noop("Connections and Authentication / Authentication"),
- /* CONN_AUTH_SSL */
- gettext_noop("Connections and Authentication / SSL"),
- /* RESOURCES_MEM */
- gettext_noop("Resource Usage / Memory"),
- /* RESOURCES_DISK */
- gettext_noop("Resource Usage / Disk"),
Re: Improve readability by using designated initializers when possible
On Tue, 27 Feb 2024 at 07:25, Michael Paquier wrote: > > On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote: > > On Mon, 26 Feb 2024 at 16:41, jian he 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 :)
Re: Improve readability by using designated initializers when possible
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 HerreraBreisgau, 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)
Re: Improve readability by using designated initializers when possible
On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote: > On Mon, 26 Feb 2024 at 16:41, jian he 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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Mon, Feb 26, 2024 at 05:00:13PM +0800, Japin Li wrote: > On Mon, 26 Feb 2024 at 16:41, jian he 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 signature.asc Description: PGP signature
Re: Improve readability by using designated initializers when possible
On Mon, 26 Feb 2024 at 16:41, jian he 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
Re: Improve readability by using designated initializers when possible
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.
Re: Improve readability by using designated initializers when possible
On Fri, 23 Feb 2024 at 02:57, Jeff Davis 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. v2-0001-Remove-unnecessary-object_classes-array.patch Description: Binary data v2-0002-Use-designated-initializer-syntax-to-improve-read.patch Description: Binary data
Re: Improve readability by using designated initializers when possible
On Fri, 2024-02-23 at 01:35 +0100, Jelte Fennema-Nio wrote: > On Thu, Feb 22, 2024, 23:46 Jeff Davis 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
Re: Improve readability by using designated initializers when possible
On Thu, Feb 22, 2024, 23:46 Jeff Davis 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.
Re: 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: > 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
