Melanie Plageman <melanieplageman@gmail.com> writes:
> On Mon, Sep 24, 2018 at 9:28 AM Elvis Pranskevichus <elprans@gmail.com>
> wrote:
>> Sorry for the long silence on this. I'm attaching a rebased and
>> cleaned-up patch with the earlier review issues addressed. I've checked
>> all relevant tests and verified manually.
> After taking a look at the updated patch, it appears to address the review
> feedback which I provided. However, the patch didn't apply cleanly for me,
> so it likely needs a quick look along with a rebase.
It looks like the one apply failure is due to the patch having corrected
a comment that somebody else already corrected. So no big deal there.
Looking through the thread, it seems like there's a pretty fundamental
design issue that hasn't been resolved, namely whether and how this ought
to interact with default_transaction_read_only. The patch as it stands
seems to be trying to implement the definition that the transmittable
variable session_read_only is equal to "!(DefaultXactReadOnly ||
RecoveryInProgress())". I doubt that that's a good idea. In the first
place, it's not at all clear that such a flag is sufficient for all
use-cases. In the second, it's somewhere between difficult and impossible
to correctly implement GUCs that are interdependent in that way; the
current patch certainly fails to do so. (It will not correctly track
intra-session changes to DefaultXactReadOnly, for instance.)
I think that we'd be better off maintaining a strict separation between
"in hot standby" and default_transaction_read_only. If there are use
cases in which people would like to reject connections based on either
one being true, let's handle that by marking them both GUC_REPORT, and
having libpq check both. (So there need to be two flavors of
target-session-attrs libpq parameter, depending on whether you want
"in hot standby" or "currently read only" to be checked.)
That puts us back to having to choose an appropriate GUC name, because
"session_read_only" isn't it :-(. Don't have any better ideas about
that than what's been discussed in the thread.
I'm also not terribly happy with the patch adding a whole new
cross-backend signaling mechanism for this; surely we don't really
need that. Perhaps it'd be sufficient to transmit SIGHUP and embed
the check for "just exited hot standby" in the handling for that?
regards, tom lane