Обсуждение: pg_logical_emit_message() misses a XLogFlush()
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
Вложения
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
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
Вложения
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
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
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
Вложения
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
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
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
Вложения
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
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
Вложения
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
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
Вложения
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
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
Вложения
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.
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
Вложения
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.
On Tue, Oct 17, 2023 at 11:57:33AM +0530, Amit Kapila wrote: > LGTM. Thanks, I've applied that, then. -- Michael