public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 2/3] Support to define custom wait events for extensions.
6+ messages / 3 participants
[nested] [flat]
* [PATCH 2/3] Support to define custom wait events for extensions.
@ 2023-06-15 03:57 Masahiro Ikeda <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Ikeda @ 2023-06-15 03:57 UTC (permalink / raw)
> Currently, only one PG_WAIT_EXTENSION event can be used as a
> wait event for extensions. Therefore, in environments with multiple
> extensions are installed, it could take time to identify bottlenecks.
"extensions are installed" should be "extensions installed".
> +#define NUM_BUILDIN_WAIT_EVENT_EXTENSION \
> + (WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - WAIT_EVENT_EXTENSION)
Should that be NUM_BUILTIN_WAIT_EVENT_EXTENSION?
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + MemoryContextAlloc(TopMemoryContext,
> + NamedExtension=
WaitEventTrancheRequestsAllocated
> + * sizeof(Named=
ExtensionWaitEventTrancheRequest));
I can't tell from reading other Postgres code when one should cast the
return value of MemoryContextAlloc(). Seems unnecessary to me.
> + if (NamedExtensionWaitEventTrancheRequestArray =3D=3D NULL)
> + {
> + NamedExtensionWaitEventTrancheRequestsAllocated =3D 16;
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + MemoryContextAlloc(TopMemoryContext,
> + NamedExtension=
WaitEventTrancheRequestsAllocated
> + * sizeof(Named=
ExtensionWaitEventTrancheRequest));
> + }
> +
> + if (NamedExtensionWaitEventTrancheRequests >=3D NamedExtensionWai=
tEventTrancheRequestsAllocated)
> + {
> + int i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);
> +
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + repalloc(NamedExtensionWaitEventTrancheRequestArr=
ay,
> + i * sizeof(NamedExtensionWaitEve=
ntTrancheRequest));
> + NamedExtensionWaitEventTrancheRequestsAllocated =3D i;
> + }
Do you think this code would look better in an if/else if?
> + int i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);
In the Postgres codebase, is an int always guaranteed to be at least 32
bits? I feel like a fixed-width type would be better for tracking the
length of the array, unless Postgres prefers the Size type.
> + Assert(strlen(tranche_name) + 1 <=3D NAMEDATALEN);
> + strlcpy(request->tranche_name, tranche_name, NAMEDATALEN);
A sizeof(request->tranche_name) would keep this code more in-sync if
size of tranche_name were to ever change, though I see sizeof
expressions in the codebase are not too common. Maybe just remove the +1
and make it less than rather than a less than or equal? Seems like it
might be worth noting in the docs of the function that the event name
has to be less than NAMEDATALEN, but maybe this is something extension
authors are inherently aware of?
---
What's the Postgres policy on the following?
for (int i =3D 0; ...)
for (i =3D 0; ...)
You are using 2 different patterns in WaitEventShmemInit() and
InitializeExtensionWaitEventTranches().
> + /*
> + * Copy the info about any named tranches into shared memory (so =
that
> + * other processes can see it), and initialize the requested wait=
events.
> + */
> + if (NamedExtensionWaitEventTrancheRequests > 0)
Removing this if would allow one less indentation level. Nothing would
have to change about the containing code either since the for loop will
then not run
> + ExtensionWaitEventCounter =3D (int *) ((char *) NamedExtensionWai=
tEventTrancheArray - sizeof(int));
^ permalink raw reply [nested|flat] 6+ messages in thread
* [PATCH 2/3] Support to define custom wait events for extensions.
@ 2023-06-15 03:57 Masahiro Ikeda <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Masahiro Ikeda @ 2023-06-15 03:57 UTC (permalink / raw)
> Currently, only one PG_WAIT_EXTENSION event can be used as a
> wait event for extensions. Therefore, in environments with multiple
> extensions are installed, it could take time to identify bottlenecks.
"extensions are installed" should be "extensions installed".
> +#define NUM_BUILDIN_WAIT_EVENT_EXTENSION \
> + (WAIT_EVENT_EXTENSION_FIRST_USER_DEFINED - WAIT_EVENT_EXTENSION)
Should that be NUM_BUILTIN_WAIT_EVENT_EXTENSION?
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + MemoryContextAlloc(TopMemoryContext,
> + NamedExtension=
WaitEventTrancheRequestsAllocated
> + * sizeof(Named=
ExtensionWaitEventTrancheRequest));
I can't tell from reading other Postgres code when one should cast the
return value of MemoryContextAlloc(). Seems unnecessary to me.
> + if (NamedExtensionWaitEventTrancheRequestArray =3D=3D NULL)
> + {
> + NamedExtensionWaitEventTrancheRequestsAllocated =3D 16;
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + MemoryContextAlloc(TopMemoryContext,
> + NamedExtension=
WaitEventTrancheRequestsAllocated
> + * sizeof(Named=
ExtensionWaitEventTrancheRequest));
> + }
> +
> + if (NamedExtensionWaitEventTrancheRequests >=3D NamedExtensionWai=
tEventTrancheRequestsAllocated)
> + {
> + int i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);
> +
> + NamedExtensionWaitEventTrancheRequestArray =3D (NamedExte=
nsionWaitEventTrancheRequest *)
> + repalloc(NamedExtensionWaitEventTrancheRequestArr=
ay,
> + i * sizeof(NamedExtensionWaitEve=
ntTrancheRequest));
> + NamedExtensionWaitEventTrancheRequestsAllocated =3D i;
> + }
Do you think this code would look better in an if/else if?
> + int i =3D pg_nextpower2_32(NamedExten=
sionWaitEventTrancheRequests + 1);
In the Postgres codebase, is an int always guaranteed to be at least 32
bits? I feel like a fixed-width type would be better for tracking the
length of the array, unless Postgres prefers the Size type.
> + Assert(strlen(tranche_name) + 1 <=3D NAMEDATALEN);
> + strlcpy(request->tranche_name, tranche_name, NAMEDATALEN);
A sizeof(request->tranche_name) would keep this code more in-sync if
size of tranche_name were to ever change, though I see sizeof
expressions in the codebase are not too common. Maybe just remove the +1
and make it less than rather than a less than or equal? Seems like it
might be worth noting in the docs of the function that the event name
has to be less than NAMEDATALEN, but maybe this is something extension
authors are inherently aware of?
---
What's the Postgres policy on the following?
for (int i =3D 0; ...)
for (i =3D 0; ...)
You are using 2 different patterns in WaitEventShmemInit() and
InitializeExtensionWaitEventTranches().
> + /*
> + * Copy the info about any named tranches into shared memory (so =
that
> + * other processes can see it), and initialize the requested wait=
events.
> + */
> + if (NamedExtensionWaitEventTrancheRequests > 0)
Removing this if would allow one less indentation level. Nothing would
have to change about the containing code either since the for loop will
then not run
> + ExtensionWaitEventCounter =3D (int *) ((char *) NamedExtensionWai=
tEventTrancheArray - sizeof(int));
^ permalink raw reply [nested|flat] 6+ messages in thread
* Wrong comment for ReplicationSlotCreate
@ 2025-12-29 13:39 Daniil Davydov <[email protected]>
2025-12-30 09:18 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Daniil Davydov @ 2025-12-29 13:39 UTC (permalink / raw)
To: Postgres hackers <[email protected]>
Hi,
I noticed that the comment for ReplicationSlotCreate function contains this
description for the "two_phase" option :
* two_phase: Allows decoding of prepared transactions. We allow this option
* to be enabled only at the slot creation time. If we allow this option
* to be changed during decoding then it is quite possible that we skip
* prepare first time because this option was not enabled. Now next time
* during getting changes, if the two_phase option is enabled it can skip
* prepare because by that time start decoding point has been moved. So the
* user will only get commit prepared.
But commit [1] introduced the ability to alter the "two_phase" option for the
replication slot. Thus, I guess that the comment mentioned above is
outdated and we should change it.
[1] 1462aad2e4474ab61174f8ab00992cd3d6d57c7b
--
Best regards,
Daniil Davydov
Attachments:
[text/x-patch] 0001-Fix-comment-for-ReplicationSlotCreate.patch (1.5K, ../../CAJDiXggZXQZ7bD0QcTizDt6us9aX6ZKK4dWxzgb5x3+TsVHjqQ@mail.gmail.com/2-0001-Fix-comment-for-ReplicationSlotCreate.patch)
download | inline diff:
From 9545671c48ee75ee7f9eed26af0902fcd3114573 Mon Sep 17 00:00:00 2001
From: Daniil Davidov <[email protected]>
Date: Mon, 29 Dec 2025 20:27:11 +0700
Subject: [PATCH] Fix comment for ReplicationSlotCreate
---
src/backend/replication/slot.c | 8 +-------
1 file changed, 1 insertion(+), 7 deletions(-)
diff --git a/src/backend/replication/slot.c b/src/backend/replication/slot.c
index 58c41d45516..310dcfcbd5f 100644
--- a/src/backend/replication/slot.c
+++ b/src/backend/replication/slot.c
@@ -369,13 +369,7 @@ IsSlotForConflictCheck(const char *name)
* name: Name of the slot
* db_specific: logical decoding is db specific; if the slot is going to
* be used for that pass true, otherwise false.
- * two_phase: Allows decoding of prepared transactions. We allow this option
- * to be enabled only at the slot creation time. If we allow this option
- * to be changed during decoding then it is quite possible that we skip
- * prepare first time because this option was not enabled. Now next time
- * during getting changes, if the two_phase option is enabled it can skip
- * prepare because by that time start decoding point has been moved. So the
- * user will only get commit prepared.
+ * two_phase: If enabled, allows decoding of prepared transactions.
* failover: If enabled, allows the slot to be synced to standbys so
* that logical replication can be resumed after failover.
* synced: True if the slot is synchronized from the primary server.
--
2.43.0
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Wrong comment for ReplicationSlotCreate
2025-12-29 13:39 Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
@ 2025-12-30 09:18 ` Chao Li <[email protected]>
2025-12-30 10:07 ` Re: Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Chao Li @ 2025-12-30 09:18 UTC (permalink / raw)
To: Daniil Davydov <[email protected]>; +Cc: Postgres hackers <[email protected]>
> On Dec 29, 2025, at 21:39, Daniil Davydov <[email protected]> wrote:
>
> Hi,
>
> I noticed that the comment for ReplicationSlotCreate function contains this
> description for the "two_phase" option :
>
> * two_phase: Allows decoding of prepared transactions. We allow this option
> * to be enabled only at the slot creation time. If we allow this option
> * to be changed during decoding then it is quite possible that we skip
> * prepare first time because this option was not enabled. Now next time
> * during getting changes, if the two_phase option is enabled it can skip
> * prepare because by that time start decoding point has been moved. So the
> * user will only get commit prepared.
>
> But commit [1] introduced the ability to alter the "two_phase" option for the
> replication slot. Thus, I guess that the comment mentioned above is
> outdated and we should change it.
>
> [1] 1462aad2e4474ab61174f8ab00992cd3d6d57c7b
>
> --
> Best regards,
> Daniil Davydov
> <0001-Fix-comment-for-ReplicationSlotCreate.patch>
The old comment claimed “We allow this option to be enabled only at the slot creation time” that is no longer true after commit 1462aad2e4474ab61174f8ab00992cd3d6d57c7b, so yes, the old comment need updating.
But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
So, I would suggest something like:
```
* two_phase: If enabled, allows decoding of prepared transactions.
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Wrong comment for ReplicationSlotCreate
2025-12-29 13:39 Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
2025-12-30 09:18 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
@ 2025-12-30 10:07 ` Daniil Davydov <[email protected]>
2025-12-31 02:32 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
0 siblings, 1 reply; 6+ messages in thread
From: Daniil Davydov @ 2025-12-30 10:07 UTC (permalink / raw)
To: Chao Li <[email protected]>; +Cc: Postgres hackers <[email protected]>
Hi,
On Tue, Dec 30, 2025 at 4:18 PM Chao Li <[email protected]> wrote:
>
> But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
>
> So, I would suggest something like:
> ```
> * two_phase: If enabled, allows decoding of prepared transactions.
> * Note that enabling this option after decoding has already advanced
> * may result in missing PREPARE records for transactions that were
> * prepared before the option was enabled.
>
> ```
Thanks for the review!
As far as I understand, if the publisher prepares a transaction and
then subscriber
tries to create a subscription, walsender will wait until the prepared
transaction is
finished (during execution of CREATE_REPLICATION_SLOT command).
We can find this logic inside the SnapBuildFindSnapshot function. Thus, we
cannot miss any PREPARE record for the created slot.
Am I missing something?
--
Best regards,
Daniil Davydov
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: Wrong comment for ReplicationSlotCreate
2025-12-29 13:39 Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
2025-12-30 09:18 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
2025-12-30 10:07 ` Re: Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
@ 2025-12-31 02:32 ` Chao Li <[email protected]>
0 siblings, 0 replies; 6+ messages in thread
From: Chao Li @ 2025-12-31 02:32 UTC (permalink / raw)
To: Daniil Davydov <[email protected]>; +Cc: Postgres hackers <[email protected]>
> On Dec 30, 2025, at 18:07, Daniil Davydov <[email protected]> wrote:
>
> Hi,
>
> On Tue, Dec 30, 2025 at 4:18 PM Chao Li <[email protected]> wrote:
>>
>> But I think the updated version is too simple. It loses the information that enabling two_phase later can result in missing PREPARE.
>>
>> So, I would suggest something like:
>> ```
>> * two_phase: If enabled, allows decoding of prepared transactions.
>> * Note that enabling this option after decoding has already advanced
>> * may result in missing PREPARE records for transactions that were
>> * prepared before the option was enabled.
>>
>> ```
>
> Thanks for the review!
>
> As far as I understand, if the publisher prepares a transaction and
> then subscriber
> tries to create a subscription, walsender will wait until the prepared
> transaction is
> finished (during execution of CREATE_REPLICATION_SLOT command).
> We can find this logic inside the SnapBuildFindSnapshot function. Thus, we
> cannot miss any PREPARE record for the created slot.
>
> Am I missing something?
>
> --
> Best regards,
> Daniil Davydov
You’re right that during CREATE_REPLICATION_SLOT, SnapBuildFindSnapshot() ensures we don’t miss PREPARE records for prepared transactions that exist at creation time.
1462aad2e introduced support for altering logical replication slot options, including two_phase, after the slot has been created. The commit comment says:
```
Changing the 'two_phase' option for a subscription from 'true' to 'false'
is permitted only when there are no pending prepared transactions
corresponding to that subscription. Otherwise, the changes of already
prepared transactions can be replicated again along with their corresponding
commit leading to duplicate data or errors.
```
true->false is not allowed, however false->true is permitted. So, I think the old comment is still possible today:
```
* Note that enabling this option after decoding has already advanced
* may result in missing PREPARE records for transactions that were
* prepared before the option was enabled.
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-12-31 02:32 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-06-15 03:57 [PATCH 2/3] Support to define custom wait events for extensions. Masahiro Ikeda <[email protected]>
2023-06-15 03:57 [PATCH 2/3] Support to define custom wait events for extensions. Masahiro Ikeda <[email protected]>
2025-12-29 13:39 Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
2025-12-30 09:18 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
2025-12-30 10:07 ` Re: Wrong comment for ReplicationSlotCreate Daniil Davydov <[email protected]>
2025-12-31 02:32 ` Re: Wrong comment for ReplicationSlotCreate Chao Li <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox