Re: pg_logical_emit_message() misses a XLogFlush()

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