Just curious to know, is promotion via function only allowed for hot-standby or works for any standby?
On Mon, Oct 15, 2018 at 7:16 AM Laurenz Albe <laurenz.a...@cybertec.at> wrote: > Michael Paquier wrote: > > > diff --git a/src/backend/access/transam/xlog.c > b/src/backend/access/transam/xlog.c > > > index 7375a78ffc..3a1f49e83a 100644 > > > --- a/src/backend/access/transam/xlog.c > > > +++ b/src/backend/access/transam/xlog.c > > > @@ -81,8 +81,6 @@ extern uint32 bootstrap_data_checksum_version; > > > /* File path names (all relative to $PGDATA) */ > > > #define RECOVERY_COMMAND_FILE "recovery.conf" > > > #define RECOVERY_COMMAND_DONE "recovery.done" > > > -#define PROMOTE_SIGNAL_FILE "promote" > > > -#define FALLBACK_PROMOTE_SIGNAL_FILE "fallback_promote" > > > > Perhaps we could just move the whole set to xlog.h. > > Done. > > > > +Datum > > > +pg_promote(PG_FUNCTION_ARGS) > > > +{ > > > + FILE *promote_file; > > > + > > > + if (!superuser()) > > > + ereport(ERROR, > > > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > > > + errmsg("must be superuser to promote > standby servers"))); > > > > Let's remove this restriction at code level, and instead use > > function-level ACLs so as it is possible to allow non-superusers to > > trigger a promotion as well, say a dedicated role. We could add a > > system role for this purpose, but I am not sure that it is worth the > > addition yet. > > Agreed, done. > > > > + while ($self->safe_psql('postgres', "SELECT pg_is_in_recovery()") > != 'f') > > > + { > > > + sleep 1; > > > + } > > > + return; > > > > This could go on infinitely, locking down buildfarm machines silently if > > a commit is not careful. What I would suggest to do instead is to not > > touch PostgresNode.pm at all, and to just modify one test to trigger > > promotion with the SQL function. Then from the test, you should add a > > comment that triggerring promotion from SQL is wanted instead of the > > internal routine, and then please add a call to poll_query_until() in > > the same way as what 6deb52b2 has removed. > > I have modified recovery/t/004_timeline_switch.pl and recovery/t/ > 009_twophase.pl > accordingly: one calls the function with "wait => true", the other > uses "wait => false" and waits as you suggested. > > > As of now, this function returns true if promotion has been triggered, > > and false if not. However it seems to me that we should have something > > more consistent with "pg_ctl promote", so there are actually three > > possible states: > > 1) Node is in recovery, with promotion triggered. pg_promote() returns > > true in case of success in creating the trigger file. > > 2) Node is in recovery, with promotion triggered. pg_promote() returns > > false in case of failure in creating the trigger file. > > 3) Node is not in recovery, where I think a call to pg_promote should > > trigger an error. This is not handled in the patch. > > So far I had returned "false" in the last case, but I am fine with > throwing an error in that case. Done. > > > An other thing which has value is to implement a "wait" mode for this > > feature, or actually a nowait mode. You could simply do what pg_ctl > > does with its logic in get_control_dbstate() by looking at the control > > file state. I think that waiting for the promotion to happen should be > > the default. > > I have implemented that. > > > At the end this patch needs a bit more work. > > Yes, it did. Thanks for the thorough review! > > Yours, > Laurenz Albe >