Re: Making WAL receiver startup rely on GUC context forprimary_conninfo and primary_slot_name

On Tue, Jan 15, 2019 at 8:48 PM Michael Paquier <michael@paquier.xyz> wrote:
> > So the answer to your question is: the WAL receiver fails to start.
>
> Robert, does this answer your question?  I agree that depending on the
> timing, we could finish by having the startup process spawning a WAL
> receiver with a given connection string, and that the WAL receiver
> could use a different one, but as long as we fail the WAL receiver
> startup this does not seem like an issue to me, so I disagree with the
> upthread statement that the patch author has not thought about such
> cases :)

OK, if that was in the patch, I dunno why I didn't see it.  I really
didn't think it was there, but if I'm wrong, then I am.

> It seems to me that making the WAL receiver relying more on the GUC
> context makes it more robust when it comes to reloading the parameters
> which would be an upcoming change as we need to rely on the WAL
> receiver check the GUC context itself and FATAL (or we would need to
> have the startup process check for a change in
> primary_conninfo/primary_slot_name, and then have the startup process
> kill the WAL receiver by itself).  In short, handling all that in the
> WAL receiver would be more robust in the long term in my opinion as we
> reduce interactions between the WAL receiver and the startup process.
> And on top of it we get rid of ready_to_display, which is something I
> am unhappy with since we had to introduce it.

I think you have some valid points here, but I also think that the
patch as written doesn't really seem to have any user-visible
benefits.  Sure, ready_to_display is a crock, but getting rid of it
doesn't immediately help anybody.  And on the flip side your patch
might have bugs, in which case we'll be worse off.  I'm not going to
stand on my soapbox and say that committing this patch is a terrible
idea, because as far as I can tell, it isn't.  But it feels like it
might be a commit for the sake of making a commit, and I can't get too
excited about that either.  Since the logic will have to be further
revised anyway to make primary_conninfo PGC_SIGHUP, why not just wait
until that's done and then commit all the changes together instead of
guessing that this might make that easier?

If this does get committed now, and count me as about -0.25 for that,
then I at least think it should have some comments clearly addressing
the synchronization issues.  Unless those are also in the patch and I
missed them, too, but I hope I'm not that dense.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company