Thread: pg_logical_emit_message() misses a XLogFlush()

pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
Hi all,

While playing with pg_logical_emit_message() and WAL replay, I have
noticed that LogLogicalMessage() inserts a record but forgets to make
sure that the record has been flushed.  So, for example, if the system
crashes the message inserted can get lost.

I was writing some TAP tests for it for the sake of a bug, and I have
found this the current behavior annoying because one cannot really
rely on it when emulating crashes.

This has been introduced in 3fe3511 (from 2016), and there is no
mention of that on the original thread that led to this commit:
https://www.postgresql.org/message-id/flat/5685F999.6010202%402ndquadrant.com

This could be an issue for anybody using LogLogicalMessage() out of
core, as well, because it would mean some records lost.  So, perhaps
this should be treated as a bug, sufficient for a backpatch?

Thoughts?
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Tomas Vondra
Date:

On 8/15/23 08:38, Michael Paquier wrote:
> Hi all,
> 
> While playing with pg_logical_emit_message() and WAL replay, I have
> noticed that LogLogicalMessage() inserts a record but forgets to make
> sure that the record has been flushed.  So, for example, if the system
> crashes the message inserted can get lost.
> 
> I was writing some TAP tests for it for the sake of a bug, and I have
> found this the current behavior annoying because one cannot really
> rely on it when emulating crashes.
> 
> This has been introduced in 3fe3511 (from 2016), and there is no
> mention of that on the original thread that led to this commit:
> https://www.postgresql.org/message-id/flat/5685F999.6010202%402ndquadrant.com
> 
> This could be an issue for anybody using LogLogicalMessage() out of
> core, as well, because it would mean some records lost.  So, perhaps
> this should be treated as a bug, sufficient for a backpatch?
> 

Shouldn't the flush be done only for non-transactional messages? The
transactional case will be flushed by regular commit flush.

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
> Shouldn't the flush be done only for non-transactional messages? The
> transactional case will be flushed by regular commit flush.

Indeed, that would be better.  I am sending an updated patch.

I'd like to backpatch that, would there be any objections to that?
This may depend on how much logical solutions depend on this routine.
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Andres Freund
Date:
Hi,

On 2023-08-16 06:58:56 +0900, Michael Paquier wrote:
> On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
> > Shouldn't the flush be done only for non-transactional messages? The
> > transactional case will be flushed by regular commit flush.
> 
> Indeed, that would be better.  I am sending an updated patch.
> 
> I'd like to backpatch that, would there be any objections to that?

Yes, I object. This would completely cripple the performance of some uses of
logical messages - a slowdown of several orders of magnitude. It's not clear
to me that flushing would be the right behaviour if it weren't released, but
it certainly doesn't seem right to make such a change in a minor release.

Greetings,

Andres Freund



Re: pg_logical_emit_message() misses a XLogFlush()

From
Tomas Vondra
Date:
On 8/16/23 02:33, Andres Freund wrote:
> Hi,
> 
> On 2023-08-16 06:58:56 +0900, Michael Paquier wrote:
>> On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
>>> Shouldn't the flush be done only for non-transactional messages? The
>>> transactional case will be flushed by regular commit flush.
>>
>> Indeed, that would be better.  I am sending an updated patch.
>>
>> I'd like to backpatch that, would there be any objections to that?
> 
> Yes, I object. This would completely cripple the performance of some uses of
> logical messages - a slowdown of several orders of magnitude. It's not clear
> to me that flushing would be the right behaviour if it weren't released, but
> it certainly doesn't seem right to make such a change in a minor release.
> 

So are you objecting to adding the flush in general, or just to the
backpatching part?

IMHO we either guarantee durability of non-transactional messages, in
which case this would be a clear bug - and I'd say a fairly serious one.
I'm curious what the workload that'd see order of magnitude of slowdown
does with logical messages, but even if such workload exists, would it
really be enough to fix any other durability bug?

Or perhaps we don't want to guarantee durability for such messages, in
which case we don't need to fix it at all (even in master).

The docs are not very clear on what to expect, unfortunately. It says
that non-transactional messages are "written immediately" which I could
interpret in either way.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Wed, Aug 16, 2023 at 03:20:53AM +0200, Tomas Vondra wrote:
> So are you objecting to adding the flush in general, or just to the
> backpatching part?
>
> IMHO we either guarantee durability of non-transactional messages, in
> which case this would be a clear bug - and I'd say a fairly serious one.
> I'm curious what the workload that'd see order of magnitude of slowdown
> does with logical messages, but even if such workload exists, would it
> really be enough to fix any other durability bug?

Yes, I also think that this is a pretty serious issue to not ensure
durability in the non-transactional case if you have solutions
designed around that.

> Or perhaps we don't want to guarantee durability for such messages, in
> which case we don't need to fix it at all (even in master).

I mean, just look at the first message of the thread I am mentioning
at the top of this thread: it lists three cases where something like
pg_logical_emit_message() was wanted.  And, it looks like a serious
issue to me for the first two ones at least (out-of-order messages and
inter-node communication), because an application may want to send
something from node1 to node2, and node1 may just forget about it
entirely if it crashes, finishing WAL redo at an earlier point than
the record inserted.

> The docs are not very clear on what to expect, unfortunately. It says
> that non-transactional messages are "written immediately" which I could
> interpret in either way.

Agreed.  The original thread never mentions "flush", "sync" or
"durab".

I won't fight much if there are objections to backpatching, but that's
not really wise (no idea how much EDB's close flavor of BDR relies on
that).
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Andres Freund
Date:
Hi,

On 2023-08-16 03:20:53 +0200, Tomas Vondra wrote:
> On 8/16/23 02:33, Andres Freund wrote:
> > Hi,
> >
> > On 2023-08-16 06:58:56 +0900, Michael Paquier wrote:
> >> On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
> >>> Shouldn't the flush be done only for non-transactional messages? The
> >>> transactional case will be flushed by regular commit flush.
> >>
> >> Indeed, that would be better.  I am sending an updated patch.
> >>
> >> I'd like to backpatch that, would there be any objections to that?
> >
> > Yes, I object. This would completely cripple the performance of some uses of
> > logical messages - a slowdown of several orders of magnitude. It's not clear
> > to me that flushing would be the right behaviour if it weren't released, but
> > it certainly doesn't seem right to make such a change in a minor release.
> >
>
> So are you objecting to adding the flush in general, or just to the
> backpatching part?

Both, I think. I don't object to adding a way to trigger flushing, but I think
it needs to be optional.


> IMHO we either guarantee durability of non-transactional messages, in which
> case this would be a clear bug - and I'd say a fairly serious one.  I'm
> curious what the workload that'd see order of magnitude of slowdown does
> with logical messages I've used it, but even if such workload exists, would
> it really be enough to fix any other durability bug?

Not sure what you mean with the last sentence?

I've e.g. used non-transactional messages for:

- A non-transactional queuing system. Where sometimes one would dump a portion
  of tables into messages, with something like
  SELECT pg_logical_emit_message(false, 'app:<task>', to_json(r)) FROM r;
  Obviously flushing after every row would be bad.

  This is useful when you need to coordinate with other systems in a
  non-transactional way. E.g. letting other parts of the system know that
  files on disk (or in S3 or ...) were created/deleted, since a database
  rollback wouldn't unlink/revive the files.

- Audit logging, when you want to log in a way that isn't undone by rolling
  back transaction - just flushing every pg_logical_emit_message() would
  increase the WAL flush rate many times, because instead of once per
  transaction, you'd now flush once per modified row. It'd basically make it
  impractical to use for such things.

- Optimistic locking. Emitting things that need to be locked on logical
  replicas, to be able to commit on the primary. A pre-commit hook would wait
  for the WAL to be replayed sufficiently - but only once per transaction, not
  once per object.


> Or perhaps we don't want to guarantee durability for such messages, in
> which case we don't need to fix it at all (even in master).

Well, I can see adding an option to flush, or perhaps a separate function to
flush, to master.


> The docs are not very clear on what to expect, unfortunately. It says
> that non-transactional messages are "written immediately" which I could
> interpret in either way.

Yea, the docs certainly should be improved, regardless what we end up with.

Greetings,

Andres Freund



Re: pg_logical_emit_message() misses a XLogFlush()

From
Andres Freund
Date:
Hi,

On 2023-08-16 12:37:21 +0900, Michael Paquier wrote:
> I won't fight much if there are objections to backpatching, but that's
> not really wise (no idea how much EDB's close flavor of BDR relies on
> that).

To be clear: I don't just object to backpatching, I also object to making
existing invocations flush WAL in HEAD. I do not at all object to adding a
parameter that indicates flushing, or a separate function to do so. The latter
might be better, because it allows you to flush a group of messages, rather
than a single one.

Greetings,

Andres Freund



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Tue, Aug 15, 2023 at 09:16:53PM -0700, Andres Freund wrote:
> To be clear: I don't just object to backpatching, I also object to making
> existing invocations flush WAL in HEAD. I do not at all object to adding a
> parameter that indicates flushing, or a separate function to do so. The latter
> might be better, because it allows you to flush a group of messages, rather
> than a single one.

For the latter, am I getting it right that you mean a function
completely outside of the scope of LogLogicalMessage() and
pg_logical_emit_message()?  Say, a single pg_wal_flush(lsn)?

I am a bit concerned by that, because anybody calling directly
LogLogicalMessage() or the existing function would never really think
about durability but they may want to ensure a message flush in their
code calling it.  Adding an argument does not do much about the SQL
function if it has a DEFAULT, still it addresses my first concern
about the C part.

Anyway, attached is a patch to add a 4th argument "flush" that
defaults to false.  Thoughts about this version are welcome.
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Tomas Vondra
Date:

On 8/16/23 06:13, Andres Freund wrote:
> Hi,
> 
> On 2023-08-16 03:20:53 +0200, Tomas Vondra wrote:
>> On 8/16/23 02:33, Andres Freund wrote:
>>> Hi,
>>>
>>> On 2023-08-16 06:58:56 +0900, Michael Paquier wrote:
>>>> On Tue, Aug 15, 2023 at 11:37:32AM +0200, Tomas Vondra wrote:
>>>>> Shouldn't the flush be done only for non-transactional messages? The
>>>>> transactional case will be flushed by regular commit flush.
>>>>
>>>> Indeed, that would be better.  I am sending an updated patch.
>>>>
>>>> I'd like to backpatch that, would there be any objections to that?
>>>
>>> Yes, I object. This would completely cripple the performance of some uses of
>>> logical messages - a slowdown of several orders of magnitude. It's not clear
>>> to me that flushing would be the right behaviour if it weren't released, but
>>> it certainly doesn't seem right to make such a change in a minor release.
>>>
>>
>> So are you objecting to adding the flush in general, or just to the
>> backpatching part?
> 
> Both, I think. I don't object to adding a way to trigger flushing, but I think
> it needs to be optional.
> 
> 
>> IMHO we either guarantee durability of non-transactional messages, in which
>> case this would be a clear bug - and I'd say a fairly serious one.  I'm
>> curious what the workload that'd see order of magnitude of slowdown does
>> with logical messages I've used it, but even if such workload exists, would
>> it really be enough to fix any other durability bug?
> 
> Not sure what you mean with the last sentence?
> 

Sorry, I meant to write "enough not to fix any other durability bug".

That is, either we must not lose messages, in which case it's a bug and
we should fix that. Or it's acceptable for the intended use cases, in
which case there's nothing to fix.

To me losing messages seems like a bad thing, but if the users are aware
of it and are fine with it ... I'm simply arguing that if we conclude
this is a durability bug, we should not leave it unfixed because it
might have performance impact.

> I've e.g. used non-transactional messages for:
> 
> - A non-transactional queuing system. Where sometimes one would dump a portion
>   of tables into messages, with something like
>   SELECT pg_logical_emit_message(false, 'app:<task>', to_json(r)) FROM r;
>   Obviously flushing after every row would be bad.
> 
>   This is useful when you need to coordinate with other systems in a
>   non-transactional way. E.g. letting other parts of the system know that
>   files on disk (or in S3 or ...) were created/deleted, since a database
>   rollback wouldn't unlink/revive the files.
> 
> - Audit logging, when you want to log in a way that isn't undone by rolling
>   back transaction - just flushing every pg_logical_emit_message() would
>   increase the WAL flush rate many times, because instead of once per
>   transaction, you'd now flush once per modified row. It'd basically make it
>   impractical to use for such things.
> 
> - Optimistic locking. Emitting things that need to be locked on logical
>   replicas, to be able to commit on the primary. A pre-commit hook would wait
>   for the WAL to be replayed sufficiently - but only once per transaction, not
>   once per object.
> 

How come the possibility of losing messages is not an issue for these
use cases? I mean, surely auditors would not like that, and I guess
forgetting locks might be bad too.

> 
>> Or perhaps we don't want to guarantee durability for such messages, in
>> which case we don't need to fix it at all (even in master).
> 
> Well, I can see adding an option to flush, or perhaps a separate function to
> flush, to master.
>>
>> The docs are not very clear on what to expect, unfortunately. It says
>> that non-transactional messages are "written immediately" which I could
>> interpret in either way.
> 
> Yea, the docs certainly should be improved, regardless what we end up with.
> 

regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Wed, Aug 16, 2023 at 12:01:01PM +0200, Tomas Vondra wrote:
> To me losing messages seems like a bad thing, but if the users are aware
> of it and are fine with it ... I'm simply arguing that if we conclude
> this is a durability bug, we should not leave it unfixed because it
> might have performance impact.

I've been doing some digging here, and the original bdr repo posted at
[1] has a concept similar to LogLogicalMessage() called
LogStandbyMessage().  *All* the non-transactional code paths enforce
an XLogFlush() after *each* message logged.  So the original
expectation seems pretty clear to me: flushes were wanted.

[1]: https://github.com/2ndQuadrant/bdr

>> I've e.g. used non-transactional messages for:
>>
>> - A non-transactional queuing system. Where sometimes one would dump a portion
>>   of tables into messages, with something like
>>   SELECT pg_logical_emit_message(false, 'app:<task>', to_json(r)) FROM r;
>>   Obviously flushing after every row would be bad.
>>
>>   This is useful when you need to coordinate with other systems in a
>>   non-transactional way. E.g. letting other parts of the system know that
>>   files on disk (or in S3 or ...) were created/deleted, since a database
>>   rollback wouldn't unlink/revive the files.
>>
>> - Audit logging, when you want to log in a way that isn't undone by rolling
>>   back transaction - just flushing every pg_logical_emit_message() would
>>   increase the WAL flush rate many times, because instead of once per
>>   transaction, you'd now flush once per modified row. It'd basically make it
>>   impractical to use for such things.
>>
>> - Optimistic locking. Emitting things that need to be locked on logical
>>   replicas, to be able to commit on the primary. A pre-commit hook would wait
>>   for the WAL to be replayed sufficiently - but only once per transaction, not
>>   once per object.
>
> How come the possibility of losing messages is not an issue for these
> use cases? I mean, surely auditors would not like that, and I guess
> forgetting locks might be bad too.

+1.  Now I can also get why one may not want to flush every individual
messages if you care only about a queue to be flushed after generating
a series of them, so after sleeping on it I'm OK with the last patch I
posted where one can just choose what he wants.  The default, though,
may be better if flush is true compared to false (the patch has kept
flush at false)..

I am not sure where this is leading yet, so I have registered a CF
entry to keep track of that:
https://commitfest.postgresql.org/44/4505/
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Fujii Masao
Date:

On 2023/08/16 16:51, Michael Paquier wrote:
> Anyway, attached is a patch to add a 4th argument "flush" that
> defaults to false.  Thoughts about this version are welcome.

When the "transactional" option is set to true, WAL including
the record generated by the pg_logical_emit_message() function is flushed
at the end of the transaction based on the synchronous_commit setting.
However, in the current patch, if "transactional" is set to false and
"flush" is true, the function flushes the WAL immediately without
considering synchronous_commit. Is this the intended behavior?
I'm not sure how the function should work in this case, though.

Though I don't understand the purpose of this option fully yet,
is flushing the WAL sufficient? Are there scenarios where the function
should ensure that the WAL is not only flushed but also replicated
to the standby?

Regards,

-- 
Fujii Masao
Advanced Computing Technology Center
Research and Development Headquarters
NTT DATA CORPORATION



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Mon, Sep 11, 2023 at 12:54:11PM +0900, Fujii Masao wrote:
> However, in the current patch, if "transactional" is set to false and
> "flush" is true, the function flushes the WAL immediately without
> considering synchronous_commit. Is this the intended behavior?
> I'm not sure how the function should work in this case, though.

Yes, that's the intended behavior.  This just offers more options to
the toolkit of this function to give more control to applications when
emitting a message.  In this case, like the current non-transactional
case, we make the record immediately available to logical decoders but
also make sure that it is flushed to disk.  If one wants to force the
record's presence to a remote instance, then using the transactional
mode would be sufficient.

Perhaps you have a point here, though, that we had better make
entirely independent the flush and transactional parts, and still
call XLogFlush() even in transactional mode.  One would make sure that
the record is on disk before waiting for the commit to do so, but
that's also awkward for applications because they would not know the
end LSN of the emitted message until the internal transaction commits
the allocated XID, which would be a few records after the result
coming out of pg_logical_emit_message().

The documentation does not worry about any of that even now in the
case of the non-transactional case, and it does not mention that one
may need to monitor pg_stat_replication or similar to make sure that
the LSN of the message exists on the remote with an application-level
check, either.  How about adding an extra paragraph to the
documentation, then?  I could think of something like that, but the
current docs also outline this a bit by telling that the message is
*not* part of a transaction, which kind of implies, at least to me,
that synchonous_commit is moot in this case:
"When transactional is false, note that the backend ignores
synchronous_commit as the record is not part of a transaction so there
is no commit to wait for. Ensuring that the record of a message
emitted exists on standbys requires additional monitoring."

> Though I don't understand the purpose of this option fully yet,
> is flushing the WAL sufficient? Are there scenarios where the function
> should ensure that the WAL is not only flushed but also replicated
> to the standby?

The flush makes sure that the record is durable, but we only care
about transaction commits in a synchronous setup, so that's
independent, in my opinion.  If you look closely, we do some fancy
stuff in finish_sync_worker(), for example, where a transaction commit
is enforced to make sure that the internal flush is sensitive to the
synchronous commit requirements, but that's just something we expect
to happen in a sync worker.
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
bt23nguyent
Date:
On 2023-09-11 14:02, Michael Paquier wrote:
> On Mon, Sep 11, 2023 at 12:54:11PM +0900, Fujii Masao wrote:
>> However, in the current patch, if "transactional" is set to false and
>> "flush" is true, the function flushes the WAL immediately without
>> considering synchronous_commit. Is this the intended behavior?
>> I'm not sure how the function should work in this case, though.
> 
> Yes, that's the intended behavior.  This just offers more options to
> the toolkit of this function to give more control to applications when
> emitting a message.  In this case, like the current non-transactional
> case, we make the record immediately available to logical decoders but
> also make sure that it is flushed to disk.  If one wants to force the
> record's presence to a remote instance, then using the transactional
> mode would be sufficient.
> 
> Perhaps you have a point here, though, that we had better make
> entirely independent the flush and transactional parts, and still
> call XLogFlush() even in transactional mode.  One would make sure that
> the record is on disk before waiting for the commit to do so, but
> that's also awkward for applications because they would not know the
> end LSN of the emitted message until the internal transaction commits
> the allocated XID, which would be a few records after the result
> coming out of pg_logical_emit_message().
> 
> The documentation does not worry about any of that even now in the
> case of the non-transactional case, and it does not mention that one
> may need to monitor pg_stat_replication or similar to make sure that
> the LSN of the message exists on the remote with an application-level
> check, either.  How about adding an extra paragraph to the
> documentation, then?  I could think of something like that, but the
> current docs also outline this a bit by telling that the message is
> *not* part of a transaction, which kind of implies, at least to me,
> that synchonous_commit is moot in this case:
> "When transactional is false, note that the backend ignores
> synchronous_commit as the record is not part of a transaction so there
> is no commit to wait for. Ensuring that the record of a message
> emitted exists on standbys requires additional monitoring."
> 
>> Though I don't understand the purpose of this option fully yet,
>> is flushing the WAL sufficient? Are there scenarios where the function
>> should ensure that the WAL is not only flushed but also replicated
>> to the standby?
> 
> The flush makes sure that the record is durable, but we only care
> about transaction commits in a synchronous setup, so that's
> independent, in my opinion.  If you look closely, we do some fancy
> stuff in finish_sync_worker(), for example, where a transaction commit
> is enforced to make sure that the internal flush is sensitive to the
> synchronous commit requirements, but that's just something we expect
> to happen in a sync worker.
> --
> Michael
Hi,

With regard to the patch, the documentation outlines the 
pg_logical_emit_message function and its corresponding syntax in the 
following manner.

pg_logical_emit_message ( transactional boolean, prefix text, content 
text ) → pg_lsn
pg_logical_emit_message ( transactional boolean, prefix text, content 
bytea [, flush boolean DEFAULT false] ) → pg_lsn

A minor issue with the description here is that while the description 
for the new flush argument in pg_logical_emit_message() with bytea type 
is clearly declared, there is no description of flush argument in the 
former pg_logical_emit_message() with text type at all.

Additionally, there is a lack of consistency in the third argument names 
between the function definition and the description (i.e., "message 
bytea" versus "<parameter>content</parameter> <type>bytea</type>") as 
follows.
----------------
+CREATE OR REPLACE FUNCTION pg_catalog.pg_logical_emit_message(
+   transactional boolean,
+   prefix text,
+   message bytea,
+   flush boolean DEFAULT false)
+RETURNS pg_lsn
+LANGUAGE INTERNAL
+VOLATILE STRICT
+AS 'pg_logical_emit_message_bytea';
----------------
...
----------------
+       <function>pg_logical_emit_message</function> ( 
<parameter>transactional</parameter> <type>boolean</type>, 
<parameter>prefix</parameter> <type>text</type>, 
<parameter>content</parameter> <type>bytea</type> [, 
<parameter>flush</parameter> <type>boolean</type> 
<literal>DEFAULT</literal> <literal>false</literal>] )
----------------
Could you please provide clarification on the reason for this 
differentiation?

On a side note, could you also include a bit more information that 
"flush is set to false by default" in the document as well? It could be 
helpful for the users.

Regards,
Tung Nguyen



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Mon, Sep 11, 2023 at 02:42:16PM +0900, bt23nguyent wrote:
> A minor issue with the description here is that while the description for
> the new flush argument in pg_logical_emit_message() with bytea type is
> clearly declared, there is no description of flush argument in the former
> pg_logical_emit_message() with text type at all.

Indeed, I forgot to update the first function signature.  Fixed in the
attached.

> On a side note, could you also include a bit more information that "flush is
> set to false by default" in the document as well? It could be helpful for
> the users.

With the function signature saying that, that did not seem stricly
necessary to me, but no objections to add a few words about that.

I'll need a bit more input from Fujii-san before doing anything about
his comments, still it looks like a doc issue to me that may need a
backpatch to clarify how the non-transactional case behaves.

Attaching a v4 with the two doc changes, fow now.
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Amit Kapila
Date:
On Mon, Sep 11, 2023 at 5:13 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> I'll need a bit more input from Fujii-san before doing anything about
> his comments, still it looks like a doc issue to me that may need a
> backpatch to clarify how the non-transactional case behaves.
>

I would prefer to associate the new parameter 'flush' with
non-transactional messages as per the proposed patch.

Few points for you to consider:
1.
+CREATE OR REPLACE FUNCTION pg_catalog.pg_logical_emit_message(
+    transactional boolean,
+    prefix text,
+    message text,
+    flush boolean DEFAULT false)
+RETURNS pg_lsn
+LANGUAGE INTERNAL
+VOLATILE STRICT
+AS 'pg_logical_emit_message_text';
+
+CREATE OR REPLACE FUNCTION pg_catalog.pg_logical_emit_message(
+    transactional boolean,
+    prefix text,
+    message bytea,
+    flush boolean DEFAULT false)
+RETURNS pg_lsn
+LANGUAGE INTERNAL
+VOLATILE STRICT

Is there a reason to make the functions strict now when they were not earlier?

2.
+        The <parameter>flush</parameter> parameter (default set to
+        <literal>false</literal>) controls if the message is immediately
+        flushed to WAL or not. <parameter>flush</parameter> has no effect
+        with <parameter>transactional</parameter>, as the message's WAL
+        record is flushed when its transaction is committed.

The last part of the message sounds a bit too specific (".. as the
message's WAL record is flushed when its transaction is committed.")
because sometimes the WAL could be flushed by walwriter even before
the commit. Can we say something along the lines: ".. as the message's
WAL record is flushed along with its transaction."?

3.
+ /*
+ * Make sure that the message hits disk before leaving if not emitting a
+ * transactional message, if flush is requested.
+ */
+ if (!transactional && flush)

Two ifs in the above comment sound a bit odd but if we want to keep it
like that then adding 'and' before the second if may slightly improve
it.

--
With Regards,
Amit Kapila.



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Fri, Oct 13, 2023 at 03:20:30PM +0530, Amit Kapila wrote:
> I would prefer to associate the new parameter 'flush' with
> non-transactional messages as per the proposed patch.

Check.

> Is there a reason to make the functions strict now when they were not earlier?

These two are already STRICT on HEAD:
=# select proname, provolatile, proisstrict from pg_proc
     where proname ~ 'message';
         proname         | provolatile | proisstrict
-------------------------+-------------+-------------
 pg_logical_emit_message | v           | t
 pg_logical_emit_message | v           | t
(2 rows)

> 2.
> +        The <parameter>flush</parameter> parameter (default set to
> +        <literal>false</literal>) controls if the message is immediately
> +        flushed to WAL or not. <parameter>flush</parameter> has no effect
> +        with <parameter>transactional</parameter>, as the message's WAL
> +        record is flushed when its transaction is committed.
>
> The last part of the message sounds a bit too specific (".. as the
> message's WAL record is flushed when its transaction is committed.")
> because sometimes the WAL could be flushed by walwriter even before
> the commit. Can we say something along the lines: ".. as the message's
> WAL record is flushed along with its transaction."?

Fine by me.

> 3.
> + /*
> + * Make sure that the message hits disk before leaving if not emitting a
> + * transactional message, if flush is requested.
> + */
> + if (!transactional && flush)
>
> Two ifs in the above comment sound a bit odd but if we want to keep it
> like that then adding 'and' before the second if may slightly improve
> it.

Sure, I've improved this comment.

An updated version is attached.  How does it look?
--
Michael

Attachment

Re: pg_logical_emit_message() misses a XLogFlush()

From
Amit Kapila
Date:
On Mon, Oct 16, 2023 at 12:47 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Fri, Oct 13, 2023 at 03:20:30PM +0530, Amit Kapila wrote:
> > I would prefer to associate the new parameter 'flush' with
> > non-transactional messages as per the proposed patch.
>
> Check.
>
> > Is there a reason to make the functions strict now when they were not earlier?
>
> These two are already STRICT on HEAD:
> =# select proname, provolatile, proisstrict from pg_proc
>      where proname ~ 'message';
>          proname         | provolatile | proisstrict
> -------------------------+-------------+-------------
>  pg_logical_emit_message | v           | t
>  pg_logical_emit_message | v           | t
> (2 rows)
>

oh, I misunderstood the default.

>
> An updated version is attached.  How does it look?
>

LGTM.

--
With Regards,
Amit Kapila.



Re: pg_logical_emit_message() misses a XLogFlush()

From
Michael Paquier
Date:
On Tue, Oct 17, 2023 at 11:57:33AM +0530, Amit Kapila wrote:
> LGTM.

Thanks, I've applied that, then.
--
Michael

Attachment