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
