[PATCH] Replace magic constant 3 with NUM_MERGE_MATCH_KINDS
Oversight of 0294df2f1f84 [1]https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84.
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patchapplication/octet-stream; name=v1-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patchDownload
From 3b19a92b4b1ef62957d9471b008ca3e4684398fa Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Apr 2024 12:37:51 +0300
Subject: [PATCH v1] Replace constant 3 with NUM_MERGE_MATCH_KINDS
Oversight of 0294df2f1f84.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: TODO FIXME
---
src/backend/optimizer/prep/prepjointree.c | 2 +-
src/backend/parser/parse_merge.c | 2 +-
src/include/nodes/execnodes.h | 2 +-
src/include/nodes/primnodes.h | 3 ++-
4 files changed, 5 insertions(+), 4 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index fb768ad52c..41da670f15 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -153,7 +153,7 @@ transform_MERGE_to_join(Query *parse)
{
RangeTblEntry *joinrte;
JoinExpr *joinexpr;
- bool have_action[3];
+ bool have_action[NUM_MERGE_MATCH_KINDS];
JoinType jointype;
int joinrti;
List *vars;
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index bce11d5956..87df79027d 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -109,7 +109,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
Query *qry = makeNode(Query);
ListCell *l;
AclMode targetPerms = ACL_NO_RIGHTS;
- bool is_terminal[3];
+ bool is_terminal[NUM_MERGE_MATCH_KINDS];
Index sourceRTI;
List *mergeActionList;
ParseNamespaceItem *nsitem;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index fa2f70b7a4..c1a65bad2a 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -545,7 +545,7 @@ typedef struct ResultRelInfo
OnConflictSetState *ri_onConflict;
/* for MERGE, lists of MergeActionState (one per MergeMatchKind) */
- List *ri_MergeActions[3];
+ List *ri_MergeActions[NUM_MERGE_MATCH_KINDS];
/* for MERGE, expr state for checking the join condition */
ExprState *ri_MergeJoinCondition;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index dafe93a4c9..eff83c15c4 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1964,7 +1964,8 @@ typedef enum MergeMatchKind
{
MERGE_WHEN_MATCHED,
MERGE_WHEN_NOT_MATCHED_BY_SOURCE,
- MERGE_WHEN_NOT_MATCHED_BY_TARGET
+ MERGE_WHEN_NOT_MATCHED_BY_TARGET,
+ NUM_MERGE_MATCH_KINDS /* must be last */
} MergeMatchKind;
typedef struct MergeAction
--
2.44.0
On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev <
aleksander@timescale.com> wrote:
Oversight of 0294df2f1f84 [1].
[1]:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84
+1. I think this change improves the code quality. I searched for
other arrays indexed by merge match kind, but found none. So this patch
seems thorough.
Thanks
Richard
On Tue, 16 Apr 2024 at 11:35, Richard Guo <guofenglinux@gmail.com> wrote:
On Tue, Apr 16, 2024 at 5:48 PM Aleksander Alekseev <aleksander@timescale.com> wrote:
Oversight of 0294df2f1f84 [1].
[1]: https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=0294df2f1f84
+1. I think this change improves the code quality. I searched for
other arrays indexed by merge match kind, but found none. So this patch
seems thorough.
Yes this makes sense, though I note that some other similar code uses
a #define rather than inserting an enum element at the end (e.g.,
NUM_ROWFILTER_PUBACTIONS).
I guess the argument against inserting an enum element at the end is
that a switch statement on the enum value might generate a compiler
warning if it didn't have a default clause.
Looking at how NUM_ROWFILTER_PUBACTIONS is defined as the last element
plus one, it might seem to be barely any better than just defining it
to be 3, since any new enum element would probably be added at the
end, requiring it to be updated in any case. But if the number of
elements were much larger, it would be much more obviously correct,
making it a good general pattern to follow. So in the interests of
code consistency, I think we should do the same here.
Regards,
Dean
Hi,
I guess the argument against inserting an enum element at the end is
that a switch statement on the enum value might generate a compiler
warning if it didn't have a default clause.
Fair point. PFA the alternative version of the patch.
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patchapplication/octet-stream; name=v2-0001-Replace-constant-3-with-NUM_MERGE_MATCH_KINDS.patchDownload
From c54e8f557098a5488e048336715a7f89ddabf93c Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 16 Apr 2024 12:37:51 +0300
Subject: [PATCH v2] Replace constant 3 with NUM_MERGE_MATCH_KINDS
Oversight of 0294df2f1f84.
Aleksander Alekseev, reviewed by Dean Rasheed, Richard Guo
Discussion: https://postgr.es/m/CAJ7c6TMsiaV5urU_Pq6zJ2tXPDwk69-NKVh4AMN5XrRiM7N+GA@mail.gmail.com
---
src/backend/optimizer/prep/prepjointree.c | 2 +-
src/backend/parser/parse_merge.c | 2 +-
src/include/nodes/execnodes.h | 2 +-
src/include/nodes/primnodes.h | 2 ++
4 files changed, 5 insertions(+), 3 deletions(-)
diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index fb768ad52c..41da670f15 100644
--- a/src/backend/optimizer/prep/prepjointree.c
+++ b/src/backend/optimizer/prep/prepjointree.c
@@ -153,7 +153,7 @@ transform_MERGE_to_join(Query *parse)
{
RangeTblEntry *joinrte;
JoinExpr *joinexpr;
- bool have_action[3];
+ bool have_action[NUM_MERGE_MATCH_KINDS];
JoinType jointype;
int joinrti;
List *vars;
diff --git a/src/backend/parser/parse_merge.c b/src/backend/parser/parse_merge.c
index bce11d5956..87df79027d 100644
--- a/src/backend/parser/parse_merge.c
+++ b/src/backend/parser/parse_merge.c
@@ -109,7 +109,7 @@ transformMergeStmt(ParseState *pstate, MergeStmt *stmt)
Query *qry = makeNode(Query);
ListCell *l;
AclMode targetPerms = ACL_NO_RIGHTS;
- bool is_terminal[3];
+ bool is_terminal[NUM_MERGE_MATCH_KINDS];
Index sourceRTI;
List *mergeActionList;
ParseNamespaceItem *nsitem;
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index fa2f70b7a4..c1a65bad2a 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -545,7 +545,7 @@ typedef struct ResultRelInfo
OnConflictSetState *ri_onConflict;
/* for MERGE, lists of MergeActionState (one per MergeMatchKind) */
- List *ri_MergeActions[3];
+ List *ri_MergeActions[NUM_MERGE_MATCH_KINDS];
/* for MERGE, expr state for checking the join condition */
ExprState *ri_MergeJoinCondition;
diff --git a/src/include/nodes/primnodes.h b/src/include/nodes/primnodes.h
index 0052c1f0ee..8fbbc6984a 100644
--- a/src/include/nodes/primnodes.h
+++ b/src/include/nodes/primnodes.h
@@ -1970,6 +1970,8 @@ typedef enum MergeMatchKind
MERGE_WHEN_NOT_MATCHED_BY_TARGET
} MergeMatchKind;
+#define NUM_MERGE_MATCH_KINDS (MERGE_WHEN_NOT_MATCHED_BY_TARGET+1)
+
typedef struct MergeAction
{
NodeTag type;
--
2.44.0
On Thu, 18 Apr 2024 at 13:00, Aleksander Alekseev
<aleksander@timescale.com> wrote:
Fair point. PFA the alternative version of the patch.
Thanks. Committed.
Regards,
Dean
Hi,
Fair point. PFA the alternative version of the patch.
Thanks. Committed.
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.
--
Best regards,
Aleksander Alekseev
Attachments:
v1-0001-Use-macro-to-define-the-number-of-enum-values.patchapplication/octet-stream; name=v1-0001-Use-macro-to-define-the-number-of-enum-values.patchDownload
From 85f3322d21230060885d626dbc5892dce90631cd Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 19 Apr 2024 12:33:52 +0300
Subject: [PATCH v1] Use macro to define the number of enum values
Refactoring in the interest of code consistency, a follow-up to 2e068db56e31.
The argument against inserting a special enum value at the end of the enum
definition is that a switch statement might generate a compiler warning unless
it has a default clause.
Aleksander Alekseev, reviewed by TODO FIXME
Discussion: https://postgr.es/m/CAJ7c6TMsiaV5urU_Pq6zJ2tXPDwk69-NKVh4AMN5XrRiM7N%2BGA%40mail.gmail.com
---
contrib/pg_stat_statements/pg_stat_statements.c | 6 +++---
src/backend/postmaster/autovacuum.c | 5 +++--
src/bin/pg_dump/pg_backup.h | 5 +++--
src/include/storage/pmsignal.h | 6 +++---
src/include/storage/procsignal.h | 6 +++---
5 files changed, 15 insertions(+), 13 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 67cec865ba..4fd65ce82d 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -125,11 +125,11 @@ typedef enum pgssStoreKind
* and this order is required in pg_stat_statements_internal().
*/
PGSS_PLAN = 0,
- PGSS_EXEC,
-
- PGSS_NUMKIND /* Must be last value of this enum */
+ PGSS_EXEC
} pgssStoreKind;
+#define PGSS_NUMKIND (PGSS_EXEC+1)
+
/*
* Hashtable key that defines the identity of a hashtable entry. We separate
* queries by user and by database even if they are otherwise identical.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index c367ede6f8..48dd1b61b9 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -245,10 +245,11 @@ typedef struct WorkerInfoData *WorkerInfo;
typedef enum
{
AutoVacForkFailed, /* failed trying to start a worker */
- AutoVacRebalance, /* rebalance the cost limits */
- AutoVacNumSignals, /* must be last */
+ AutoVacRebalance /* rebalance the cost limits */
} AutoVacuumSignal;
+#define AutoVacNumSignals (AutoVacRebalance+1)
+
/*
* Autovacuum workitem array, stored in AutoVacuumShmem->av_workItems. This
* list is mostly protected by AutovacuumLock, except that if an item is
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index fbf5f1c515..1a03a8a27e 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -73,10 +73,11 @@ enum _dumpPreparedQueries
PREPQUERY_DUMPRANGETYPE,
PREPQUERY_DUMPTABLEATTACH,
PREPQUERY_GETCOLUMNACLS,
- PREPQUERY_GETDOMAINCONSTRAINTS,
- NUM_PREP_QUERIES /* must be last */
+ PREPQUERY_GETDOMAINCONSTRAINTS
};
+#define NUM_PREP_QUERIES (PREPQUERY_GETDOMAINCONSTRAINTS+1)
+
/* Parameters needed by ConnectDatabase; same for dump and restore */
typedef struct _connParams
{
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 029b720109..8189a39205 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -39,11 +39,11 @@ typedef enum
PMSIGNAL_START_AUTOVAC_WORKER, /* start an autovacuum worker */
PMSIGNAL_BACKGROUND_WORKER_CHANGE, /* background worker state change */
PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
- PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
-
- NUM_PMSIGNALS /* Must be last value of enum! */
+ PMSIGNAL_ADVANCE_STATE_MACHINE /* advance postmaster's state machine */
} PMSignalReason;
+#define NUM_PMSIGNALS (PMSIGNAL_ADVANCE_STATE_MACHINE+1)
+
/*
* Reasons why the postmaster would send SIGQUIT to its children.
*/
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 7d290ea7d0..553aec66d3 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -46,11 +46,11 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_LOGICALSLOT,
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
- PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
-
- NUM_PROCSIGNALS /* Must be last! */
+ PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK
} ProcSignalReason;
+#define NUM_PROCSIGNALS (PROCSIG_RECOVERY_CONFLICT_LAST+1)
+
typedef enum
{
PROCSIGNAL_BARRIER_SMGRRELEASE, /* ask smgr to close files */
--
2.44.0
On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote:
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.
I don't see why not for the places you are changing here, we can be
more consistent. Now, such changes are material for v18.
--
Michael
On Mon, 22 Apr 2024 at 06:04, Michael Paquier <michael@paquier.xyz> wrote:
On Fri, Apr 19, 2024 at 12:47:43PM +0300, Aleksander Alekseev wrote:
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.I don't see why not for the places you are changing here, we can be
more consistent.
[Shrug] I do prefer using a macro. Adding a counter element to the end
of the enum feels like a hack, because the counter isn't the same kind
of thing as all the other enum elements, so it feels out of place in
the enum. On the other hand, I think it's a fairly common pattern that
most people will recognise, and for other enums that are more likely
to grow over time, it might be less error-prone than a macro, which
people might overlook and fail to update.
Now, such changes are material for v18.
Agreed. This has been added to the next commitfest, so let's see what
others think.
Regards,
Dean
On 19.04.24 11:47, Aleksander Alekseev wrote:
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.
I think this is a sensible improvement.
But please keep the trailing commas on the last enum items.
Hi,
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.I think this is a sensible improvement.
But please keep the trailing commas on the last enum items.
Thanks, fixed.
--
Best regards,
Aleksander Alekseev
Attachments:
v2-0001-Use-macro-to-define-the-number-of-enum-values.patchapplication/octet-stream; name=v2-0001-Use-macro-to-define-the-number-of-enum-values.patchDownload
From 68373f77c42cc9cae6e7872abaa131fc6697cb91 Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Fri, 19 Apr 2024 12:33:52 +0300
Subject: [PATCH v2] Use macro to define the number of enum values
Refactoring in the interest of code consistency, a follow-up to 2e068db56e31.
The argument against inserting a special enum value at the end of the enum
definition is that a switch statement might generate a compiler warning unless
it has a default clause.
Aleksander Alekseev, reviewed by Michael Paquier, Dean Rasheed, Peter Eisentraut
Discussion: https://postgr.es/m/CAJ7c6TMsiaV5urU_Pq6zJ2tXPDwk69-NKVh4AMN5XrRiM7N%2BGA%40mail.gmail.com
---
contrib/pg_stat_statements/pg_stat_statements.c | 4 ++--
src/backend/postmaster/autovacuum.c | 3 ++-
src/bin/pg_dump/pg_backup.h | 3 ++-
src/include/storage/pmsignal.h | 4 ++--
src/include/storage/procsignal.h | 4 ++--
5 files changed, 10 insertions(+), 8 deletions(-)
diff --git a/contrib/pg_stat_statements/pg_stat_statements.c b/contrib/pg_stat_statements/pg_stat_statements.c
index 67cec865ba..0a3f569abf 100644
--- a/contrib/pg_stat_statements/pg_stat_statements.c
+++ b/contrib/pg_stat_statements/pg_stat_statements.c
@@ -126,10 +126,10 @@ typedef enum pgssStoreKind
*/
PGSS_PLAN = 0,
PGSS_EXEC,
-
- PGSS_NUMKIND /* Must be last value of this enum */
} pgssStoreKind;
+#define PGSS_NUMKIND (PGSS_EXEC+1)
+
/*
* Hashtable key that defines the identity of a hashtable entry. We separate
* queries by user and by database even if they are otherwise identical.
diff --git a/src/backend/postmaster/autovacuum.c b/src/backend/postmaster/autovacuum.c
index 9a925a10cd..a9eb4967aa 100644
--- a/src/backend/postmaster/autovacuum.c
+++ b/src/backend/postmaster/autovacuum.c
@@ -246,9 +246,10 @@ typedef enum
{
AutoVacForkFailed, /* failed trying to start a worker */
AutoVacRebalance, /* rebalance the cost limits */
- AutoVacNumSignals, /* must be last */
} AutoVacuumSignal;
+#define AutoVacNumSignals (AutoVacRebalance+1)
+
/*
* Autovacuum workitem array, stored in AutoVacuumShmem->av_workItems. This
* list is mostly protected by AutovacuumLock, except that if an item is
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index fbf5f1c515..1f43891a7d 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -74,9 +74,10 @@ enum _dumpPreparedQueries
PREPQUERY_DUMPTABLEATTACH,
PREPQUERY_GETCOLUMNACLS,
PREPQUERY_GETDOMAINCONSTRAINTS,
- NUM_PREP_QUERIES /* must be last */
};
+#define NUM_PREP_QUERIES (PREPQUERY_GETDOMAINCONSTRAINTS+1)
+
/* Parameters needed by ConnectDatabase; same for dump and restore */
typedef struct _connParams
{
diff --git a/src/include/storage/pmsignal.h b/src/include/storage/pmsignal.h
index 029b720109..88726ea4ae 100644
--- a/src/include/storage/pmsignal.h
+++ b/src/include/storage/pmsignal.h
@@ -40,10 +40,10 @@ typedef enum
PMSIGNAL_BACKGROUND_WORKER_CHANGE, /* background worker state change */
PMSIGNAL_START_WALRECEIVER, /* start a walreceiver */
PMSIGNAL_ADVANCE_STATE_MACHINE, /* advance postmaster's state machine */
-
- NUM_PMSIGNALS /* Must be last value of enum! */
} PMSignalReason;
+#define NUM_PMSIGNALS (PMSIGNAL_ADVANCE_STATE_MACHINE+1)
+
/*
* Reasons why the postmaster would send SIGQUIT to its children.
*/
diff --git a/src/include/storage/procsignal.h b/src/include/storage/procsignal.h
index 7d290ea7d0..c2b025b956 100644
--- a/src/include/storage/procsignal.h
+++ b/src/include/storage/procsignal.h
@@ -47,10 +47,10 @@ typedef enum
PROCSIG_RECOVERY_CONFLICT_BUFFERPIN,
PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
PROCSIG_RECOVERY_CONFLICT_LAST = PROCSIG_RECOVERY_CONFLICT_STARTUP_DEADLOCK,
-
- NUM_PROCSIGNALS /* Must be last! */
} ProcSignalReason;
+#define NUM_PROCSIGNALS (PROCSIG_RECOVERY_CONFLICT_LAST+1)
+
typedef enum
{
PROCSIGNAL_BARRIER_SMGRRELEASE, /* ask smgr to close files */
--
2.45.0
On 13.05.24 12:22, Aleksander Alekseev wrote:
Hi,
Thanks. I see a few pieces of code that use special FOO_NUMBER enum
values instead of a macro. Should we refactor these pieces
accordingly? PFA another patch.I think this is a sensible improvement.
But please keep the trailing commas on the last enum items.
Thanks, fixed.
committed