Re: Proposal for changes to recovery.conf API

From: Abhijit Menon-Sen <ams(at)2ndQuadrant(dot)com>
To: pgsql-hackers(at)postgresql(dot)org
Cc: Michael Paquier <michael(dot)paquier(at)gmail(dot)com>, Simon Riggs <simon(at)2ndquadrant(dot)com>, Fujii Masao <masao(dot)fujii(at)gmail(dot)com>
Subject: Re: Proposal for changes to recovery.conf API
Date: 2016-09-06 05:18:15
Message-ID: 20160906051814.GA16820@toroid.org
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi.

Here's an updated version of my patch, which now applies on top of the
patch that Simon posted earlier (recovery_startup_r10_api.v1b.patch).

A few notes:

1. I merged in recovery_target_lsn as a new GUC setting.
2. I fixed various minor nits in the earlier patch, not worth mentioning
individually.
3. I haven't added back trigger_file, because Simon's patch removes it.
I can add it back in separately after discussion (otherwise Simon's
and my patches will conflict).
4. I've tested this to the extent that setting things in postgresql.conf
works, and recovery.conf is still read if it exists, and so on.

One open issue is the various assign_recovery_target_xxx functions,
which Michael noted in his earlier review:

+static void
+assign_recovery_target_xid(const char *newval, void *extra)
+{
+ recovery_target_xid = *((TransactionId *) extra);
+
+ if (recovery_target_xid != InvalidTransactionId)
+ recovery_target = RECOVERY_TARGET_XID;
+ else if (recovery_target_name && *recovery_target_name)
+ recovery_target = RECOVERY_TARGET_NAME;
+ else if (recovery_target_time != 0)
+ recovery_target = RECOVERY_TARGET_TIME;
+ else if (recovery_target_lsn != 0)
+ recovery_target = RECOVERY_TARGET_LSN;
+ else
+ recovery_target = RECOVERY_TARGET_UNSET;
+}

(Note how recovery_target_lsn caused this—and three other functions
besides—to grow an extra branch.)

I don't like this code, but I'm not yet sure what to replace it with. I
think we should address the underlying problem—that the UI doesn't map
cleanly to what the code wants. There's been some discussion about this
earlier, but not any consensus that I could see.

Do we want something like this (easy to implement and document, perhaps
not especially convenient to use):

recovery_target = 'xid' # or 'time'/'name'/'lsn'/'immediate'
recovery_target_xid = xxx? # the only setting we care about now
recovery_target_otherthings = parsed_but_ignored

Or something like this (a bit harder to implement):

recovery_target = 'xid:xxx' # or 'time:xxx' etc.

Alternatively, the do-nothing option is to move the tests from guc.c to
StartupXLOG and do it only once in some defined order (which would also
break the current last-mention-wins behaviour).

Thoughts? (I've added Fujii to the Cc: list, in case he has any
comments, since this is based on his earlier patch.)

-- Abhijit

Attachment Content-Type Size
recovery_guc_v20160906.patch text/x-diff 115.6 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Michael Paquier 2016-09-06 05:40:54 Re: Proposal for changes to recovery.conf API
Previous Message Victor Wagner 2016-09-06 05:03:49 Re: Patch: Implement failover on libpq connect level.