Thanks for the comments Greg, please find my comments inline below.
On Tue, Feb 9, 2021 at 2:27 PM Greg Nancarrow <gregn4422@gmail.com> wrote:
>
> On Mon, Feb 8, 2021 at 8:17 PM vignesh C <vignesh21@gmail.com> wrote:
> >
> > >
> > > I think what we want to do is mark default_transaction_read_only as
> > > GUC_REPORT, instead. That will give a reliable report of what the
> > > state of its GUC is, and you can combine it with is_hot_standby
> > > to decide whether the session should be considered read-only.
> > > If you don't get those two GUC values during connection, then you
> > > can fall back on "SHOW transaction_read_only".
> > >
> >
> > I have made a patch for the above with the changes suggested and
> > rebased it with the head code.
> > Attached v21 patch which has the changes for the same.
> > Thoughts?
>
> Further to my other doc change feedback, I can only spot the following
> minor things (otherwise the changes that you have made seek OK to me).
>
> 1) doc/src/sgml/protocol.sgml
>
> <varname>default_transaction_read_only</varname> and
> <varname>in_hot_standby</varname> were not reported by releases before
> 14.)
>
> should be:
>
> <varname>default_transaction_read_only</varname> and
> <varname>in_hot_standby</varname> were not reported by releases before
> 14.0)
>
Modified.
> 2) doc/src/sgml/high-availability,sgml
>
> <para>
> During hot standby, the parameter <varname>in_hot_standby</varname> and
> <varname>default_transaction_read_only</varname> are always true and may
> not be changed.
>
> should be:
>
> <para>
> During hot standby, the parameters <varname>in_hot_standby</varname> and
> <varname>transaction_read_only</varname> are always true and may
> not be changed.
>
>
> [I believe that there's only checks on attempts to change
> "transaction_read_only" when in hot_standby, not
> "default_transaction_read_only"; see check_transaction_read_only()]
>
Modified.
> 3) src/interfaces/libpq/fe-connect.c
>
> In rejectCheckedReadOrWriteConnection() and
> rejectCheckedStandbyConnection(), now that host and port info are
> emitted separately and are not included in each error message string
> (as parameters in a format string), I think those functions should use
> appendPQExpBufferStr() instead of appendPQExpBuffer(), as it's more
> efficient if there is just a single string argument.
>
Modified.
These comments are handled in v22 patch posted in my earlier mail.
Regards,
VIgnesh
