Re: Allowing pg_recvlogical to create temporary replication slots

Hi,

This is my very first message to this mailing list. Please advise if I am
making any mistakes in the procedure.

The attached patch enables pg_recvlogical to create a temporary slot.

What is the next step in the process to get this change into official
postgres?

Thanks,
Torsten

On 27.10.24 13:37, Torsten Förtsch wrote:
> This is my very first message to this mailing list. Please advise if I
> am making any mistakes in the procedure.

Welcome!

> The attached patch enables pg_recvlogical to create a temporary slot.
>
> What is the next step in the process to get this change into official
> postgres?

I think you should explain a bit why you want to do that, what use case
or scenario this is meant to address.

On Wed, Oct 30, 2024 at 09:00:59AM +0100, Peter Eisentraut wrote:
> I think you should explain a bit why you want to do that, what use case or
> scenario this is meant to address.

Indeed. Note also this page giving a couple more guidelines with more
details:
https://wiki.postgresql.org/wiki/Submitting_a_Patch

We also use what we call a "commit fest" to keep a track of the
patches under review for the current development cycle. It is a
process where folks can register for patch reviews and provide
feedback. There is still time to register what you have here for the
commit fest beginning on the 1st of November:
https://commitfest.postgresql.org/50/
--
Michael

On Wed, Oct 30, 2024 at 9:01 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
wrote:

> On 27.10.24 13:37, Torsten Förtsch wrote:
> > The attached patch enables pg_recvlogical to create a temporary slot.
>
> I think you should explain a bit why you want to do that, what use case
> or scenario this is meant to address.
>

In my particular case I want to filter for messages sent by
pg_logical_emit_message(). I don't care much about getting ALL the changes.
If the replication slot disappears and a few changes are lost it does not
matter. So, a temporary rep slot makes more sense than creating one and
then having to make sure it is not retaining wal forever later.

I can imagine this also as a tool to monitor changes for a while and then
simply disconnect without the need to remove the slot.

Why am I interested in pg_logical_emit_message()? We have an application
with relatively complicated transactions involving multiple tables. Some of
them use pg_notify(). We also have synchronous replication. Sometimes I see
lock avalanches that can be traced to the "AccessExclusiveLock on object 0
of class 1262 of database 0". This lock is taken during commit when the
notification is inserted in the queue. After that the backend waits for the
confirmation by the sync replica. So, this lock presses all the
transactions sending notifications into a sequence.

Now, if the application uses pg_logical_emit_message() instead, then I
think there is no equivalent lock. I understand the semantics are a bit
different (timing) but close enough for my use case. Another advantage of
pg_logical_emit_message() is the ability to send them even if the
transaction is aborted.

I was in the process of experimenting with this idea and found that
pg_recvlogical can:
- only create the slot or
- create the slot and immediately use it
- try to create the slot and if the slot is already there use it

So, why not also allow it to create a temporary slot?

This is another version of the patch. It now includes tests.

On Fri, Nov 1, 2024 at 1:42 PM Torsten Förtsch <tfoertsch123(at)gmail(dot)com>
wrote:

> On Wed, Oct 30, 2024 at 9:01 AM Peter Eisentraut <peter(at)eisentraut(dot)org>
> wrote:
>
>> On 27.10.24 13:37, Torsten Förtsch wrote:
>> > The attached patch enables pg_recvlogical to create a temporary slot.
>>
>> I think you should explain a bit why you want to do that, what use case
>> or scenario this is meant to address.
>>
>
> In my particular case I want to filter for messages sent by
> pg_logical_emit_message(). I don't care much about getting ALL the changes.
> If the replication slot disappears and a few changes are lost it does not
> matter. So, a temporary rep slot makes more sense than creating one and
> then having to make sure it is not retaining wal forever later.
>
> I can imagine this also as a tool to monitor changes for a while and then
> simply disconnect without the need to remove the slot.
>
> Why am I interested in pg_logical_emit_message()? We have an application
> with relatively complicated transactions involving multiple tables. Some of
> them use pg_notify(). We also have synchronous replication. Sometimes I see
> lock avalanches that can be traced to the "AccessExclusiveLock on object 0
> of class 1262 of database 0". This lock is taken during commit when the
> notification is inserted in the queue. After that the backend waits for the
> confirmation by the sync replica. So, this lock presses all the
> transactions sending notifications into a sequence.
>
> Now, if the application uses pg_logical_emit_message() instead, then I
> think there is no equivalent lock. I understand the semantics are a bit
> different (timing) but close enough for my use case. Another advantage of
> pg_logical_emit_message() is the ability to send them even if the
> transaction is aborted.
>
> I was in the process of experimenting with this idea and found that
> pg_recvlogical can:
> - only create the slot or
> - create the slot and immediately use it
> - try to create the slot and if the slot is already there use it
>
> So, why not also allow it to create a temporary slot?
>

On Fri, Nov 01, 2024 at 01:42:55PM +0100, Torsten Förtsch wrote:
> I was in the process of experimenting with this idea and found that
> pg_recvlogical can:
> - only create the slot or
> - create the slot and immediately use it
> - try to create the slot and if the slot is already there use it
>
> So, why not also allow it to create a temporary slot?

I have no objections. I don't really have an argument regarding the
use cases for specific applications, but I can see myself use this
option for development purposes: I have some experience using
pg_recvlogical for debugging purposes with logical decoding plugins in
the past, requiring always prior replication slot creation.

I can also imagine someone willing to use it on a live server and make
sure that there is no replication slot left behind once the tool has
disconnected on the receiver side.
--
Michael

On Fri, Nov 1, 2024 at 6:13 PM Torsten Förtsch <tfoertsch123(at)gmail(dot)com> wrote:
>
> On Wed, Oct 30, 2024 at 9:01 AM Peter Eisentraut <peter(at)eisentraut(dot)org> wrote:
>>
>> On 27.10.24 13:37, Torsten Förtsch wrote:
>> > The attached patch enables pg_recvlogical to create a temporary slot.
>>
>> I think you should explain a bit why you want to do that, what use case
>> or scenario this is meant to address.
>
>
> In my particular case I want to filter for messages sent by pg_logical_emit_message(). I don't care much about getting ALL the changes. If the replication slot disappears and a few changes are lost it does not matter. So, a temporary rep slot makes more sense than creating one and then having to make sure it is not retaining wal forever later.
>
> I can imagine this also as a tool to monitor changes for a while and then simply disconnect without the need to remove the slot.
>

Your use case sounds reasonable to me. So, +1 for adding an option for
temporary slots.

--
With Regards,
Amit Kapila.

Hello Torsten!

While reviewing your patch for pg_recvlogical, I’m curious about the
expected behavior when using --if-not-exists in conjunction with --temporary-slot.

If an existing slot is reused under these options, will it behave as
a temporary slot and be removed when the connection ends? Or the
existing slot should remain persistent despite the --temporary-slot flag?

--
Dickson S. Guedes

Hi Dickson,

>
> While reviewing your patch for pg_recvlogical, I’m curious about the
> expected behavior when using --if-not-exists in conjunction with
> --temporary-slot.
>
> If an existing slot is reused under these options, will it behave as
> a temporary slot and be removed when the connection ends? Or the
> existing slot should remain persistent despite the --temporary-slot flag?
>

If the slot did not exist before, it is created as a temporary slot. If
subsequently the connection drops and pg_recvlogical reconnects (no -n
option), the slot is created again as a temporary slot. That means changes
happening in between will not be captured.

If the slot existed before, the existing slot will be used, no error is
generated. In that case the slot will keep existing when the connection is
dropped.

The following situation can happen:
- the slot does not exist and is created as a temporary slot
- the connection drops and pg_recvlogical tries to reconnect after a few
seconds
- in that period a permanent slot by the same name is created but no reader
is connected to it
- pg_recvlogical reconnects and finds the existing slot

In this case, the now existing slot is simply used. It will remain when the
connection is dropped.

Thanks for reviewing.

Basically, --if-not-exists flag tells pg_recvlogical to ignore the error it
would get when creating a slot with a name that's already there. The new
flag does not change that.

This is the relevant code in src/bin/pg_basebackup/streamutil.c checking
the result of the CREATE_REPLICATION_SLOT command.

if (slot_exists_ok &&
sqlstate &&
strcmp(sqlstate, ERRCODE_DUPLICATE_OBJECT) == 0)
{
...
return true;
}

Em ter., 12 de nov. de 2024 às 07:54, Torsten Förtsch
<tfoertsch123(at)gmail(dot)com> escreveu:
> Hi Dickson,
>
> [ ... ]
>
> If the slot existed before, the existing slot will be used, no error is generated. In that case the slot will keep existing when the connection is dropped.
>
> The following situation can happen:
> - the slot does not exist and is created as a temporary slot
> - the connection drops and pg_recvlogical tries to reconnect after a few seconds
> - in that period a permanent slot by the same name is created but no reader is connected to it
> - pg_recvlogical reconnects and finds the existing slot
>
> In this case, the now existing slot is simply used. It will remain when the connection is dropped.
>
> Thanks for reviewing.

Thank you for your clarification.

Are you planning to add changes in docs? Would be important to have
this clarification in there also, IMHO.

Regards.
--
Dickson S. Guedes

On Tue, Nov 12, 2024 at 3:30 PM Dickson S. Guedes <guedes(at)guedesoft(dot)net>
wrote:

Are you planning to add changes in docs? Would be important to have
> this clarification in there also, IMHO.
>

Thanks. Here is an updated version of the patch including documentation.

On Sat, Nov 16, 2024 at 12:49:05PM +0100, Torsten Förtsch wrote:
> Thanks. Here is an updated version of the patch including documentation.

+ <para>
+ If used together with <option>--if-not-exists</option> and if a
+ permanent slot by the requested name exists, that slot is used instead
+ of creating a new one. That permanent slot is then not automatically
+ removed when the connection is dropped.
+ </para>

I'm wondering if this is the best behavior to have. Wouldn't it be
simpler to just error out if combining --temporary-slot and
--if-not-exists altogether? For example, this part of the
documentation is not true if --if-not-exists bumps on an existing
temporary slot, no?

Another thing that is confusing is the fact of always requiring
--create-slot when using the new option. Here is a simpler idea:
if using --temporary-slot, imply that --create-slot is set then use a
slot name based on the PID of the remote connection, like
pg_basebackup (see call of PQbackendPID() in pg_basebackup.c). If
--create-slot and --temporary-slot are both specified, error.

My point is, do we actually need to care about the name of the slot
for monitoring purposes as we know this is a slot that's going to be
thrown away on disconnect? Probably not, so we could just use a
pg_recvlogical_%u.

Enforcing a permanent slot if --slot is set may not be the best thing
to do, so I'd make things simpler and issue an error if attempting to
use --slot and --temporary-slot in this case. This makes for less
cases to think about when creating the slot once the connection to the
remote is done.

+ the slot will be recreated first. If at that time another slot by the
+ same name exists, creation will fail.

And concurrency issues like this one are very unlikely going to happen
once you do that.
--
Michael

On Tue, Nov 19, 2024 at 09:53:42AM +0900, Michael Paquier wrote:
> And concurrency issues like this one are very unlikely going to happen
> once you do that.

The patch has been waiting for input for 8~9 weeks, so I am marking it
as returned with feedback in the CF app. Feel free to resubmit if you
are planning to work more on this patch.
--
Michael