Обсуждение: [HACKERS] Patch: add --if-exists to pg_recvlogical
Hackers,
While doing some scripting around pg_recvlogical at $work, I found a need for $subject. Attached, find a patch to that effect.
I tried simply to mirror the logic used elsewhere. I don't think there's anything controversial here, but welcome any comments or suggestions.
This applies and builds successfully against master, and behaves as designed (i.e., dropping or trying to stream from a nonexistent slot exits cleanly). It doesn't affect any other behavior I could observe.
If accepted, this will likely need a pgindent run upon merging; I had to give up on the rabbit hole of getting that working locally.
Thanks,
rls
--
:wq
Вложения
On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz <rosser.schwarz@gmail.com> wrote: > While doing some scripting around pg_recvlogical at $work, I found a need > for $subject. Attached, find a patch to that effect. > > I tried simply to mirror the logic used elsewhere. I don't think there's > anything controversial here, but welcome any comments or suggestions. > > This applies and builds successfully against master, and behaves as designed > (i.e., dropping or trying to stream from a nonexistent slot exits cleanly). > It doesn't affect any other behavior I could observe. > > If accepted, this will likely need a pgindent run upon merging; I had to > give up on the rabbit hole of getting that working locally. Please add this to commitfest.postgresql.org -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Fri, Aug 25, 2017 at 12:34 PM, Robert Haas <robertmhaas@gmail.com> wrote:
--
On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz
<rosser.schwarz@gmail.com> wrote:
> While doing some scripting around pg_recvlogical at $work, I found a need
> for $subject. Attached, find a patch to that effect...
Please add this to commitfest.postgresql.org
Done, thanks!
:wq
On 8/26/17 15:40, Rosser Schwarz wrote: > On Fri, Aug 25, 2017 at 12:34 PM, Robert Haas <robertmhaas@gmail.com > <mailto:robertmhaas@gmail.com>> wrote: > > On Sun, May 21, 2017 at 3:04 PM, Rosser Schwarz > <rosser.schwarz@gmail.com <mailto:rosser.schwarz@gmail.com>> wrote: > > While doing some scripting around pg_recvlogical at $work, I found a need > > for $subject. Attached, find a patch to that effect... > > Please add this to commitfest.postgresql.org > <http://commitfest.postgresql.org> > > > Done, thanks! > > https://commitfest.postgresql.org/14/1256/ <varlistentry> + <term><option>--if-exists</option></term> + <listitem> + <para> + Do not error out when <option>--drop-slot</option> or <option>--start<option> are + specified and a slot with the specified name does not exist. + </para> + </listitem> + </varlistentry> I understand the --drop-slot part. But I don't understand what it means to ignore a missing replication slot when running --start. The same option should be added to pg_receivewal as well. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Sat, Sep 2, 2017 at 5:22 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > <varlistentry> > + <term><option>--if-exists</option></term> > + <listitem> > + <para> > + Do not error out when <option>--drop-slot</option> or > <option>--start<option> are > + specified and a slot with the specified name does not exist. > + </para> > + </listitem> > + </varlistentry> > > I understand the --drop-slot part. But I don't understand what it means > to ignore a missing replication slot when running --start. Also "<option>--start<option>" breaks the documentation build (missing slash on the closing tag). -- Thomas Munro http://www.enterprisedb.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
> On 09 Sep 2017, at 06:05, Thomas Munro <thomas.munro@enterprisedb.com> wrote: > > On Sat, Sep 2, 2017 at 5:22 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> <varlistentry> >> + <term><option>--if-exists</option></term> >> + <listitem> >> + <para> >> + Do not error out when <option>--drop-slot</option> or >> <option>--start<option> are >> + specified and a slot with the specified name does not exist. >> + </para> >> + </listitem> >> + </varlistentry> >> >> I understand the --drop-slot part. But I don't understand what it means >> to ignore a missing replication slot when running --start. > > Also "<option>--start<option>" breaks the documentation build (missing > slash on the closing tag). This patch is "Waiting for Author” due to the above review comments from Peter and Thomas. Do you think you will have time to address these shortly so we can move this patch further in the process? cheers ./daniel -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 15, 2017 at 04:15 Daniel Gustafsson <daniel@yesql.se> wrote:
This patch is "Waiting for Author” due to the above review comments from Peter
and Thomas. Do you think you will have time to address these shortly so we can
move this patch further in the process?
I have a patch addressing both comments that I'm testing and tweaking now (well, not *now* now), which I hope to submit this weekend.
Is that enough time?
--rosser
--
:wq
On 9/15/17 13:18, Rosser Schwarz wrote: > On Fri, Sep 15, 2017 at 04:15 Daniel Gustafsson <daniel@yesql.se > <mailto:daniel@yesql.se>> wrote: > > This patch is "Waiting for Author” due to the above review comments > from Peter > and Thomas. Do you think you will have time to address these > shortly so we can > move this patch further in the process? > > > I have a patch addressing both comments that I'm testing and tweaking > now (well, not *now* now), which I hope to submit this weekend. > > Is that enough time? There is no rush of any kind. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 1, 2017 at 10:22 AM, Peter Eisentraut <peter.eisentraut@2ndquadrant.
> I understand the --drop-slot part. But I don't understand what it means
> to ignore a missing replication slot when running --start.
I'm not sure I do either, honestly. I followed the Principle of Least Surprise in making it a no-op when those switches are used and the slot doesn't exist, over "no one will ever do that". Because someone will.
I'm happy to hear suggestions on what to do in that case beyond exit cleanly.
> The same option should be added to pg_receivewal as well.
Done.
On Fri, Sep 8, 2017 at 21:06 Thomas Munro <thomas.munro@enterprisedb.com
> Also "<option>--start<option>" breaks the documentation build
> (missing slash on the closing tag).
Fixed, thanks.
Please see revised patch, attached, addressing both comments.
rls
:wq
Вложения
On 9/17/17 18:21, Rosser Schwarz wrote: > On Fri, Sep 1, 2017 at 10:22 AM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com > <mailto:peter.eisentraut@2ndquadrant.com>> wrote: >> I understand the --drop-slot part. But I don't understand what it means >> to ignore a missing replication slot when running --start. > > I'm not sure I do either, honestly. I followed the Principle of Least > Surprise in making it a no-op when those switches are used and the slot > doesn't exist, over "no one will ever do that". Because someone will. > > I'm happy to hear suggestions on what to do in that case beyond exit > cleanly. Nonsensical option combinations should result in an error. It appears that you have removed the interaction of --start and --if-exists in your last patch, but the documentation patch still mentions it. Which is correct? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Tue, Sep 19, 2017 at 1:12 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote:
--
On 9/17/17 18:21, Rosser Schwarz wrote:
> On Fri, Sep 1, 2017 at 10:22 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com
> <mailto:peter.eisentraut@2ndquadrant.com>> wrote:
>> I understand the --drop-slot part. But I don't understand what it means
>> to ignore a missing replication slot when running --start
> I'm not sure I do either, honestly. I followed the Principle of Least
> Surprise in making it a no-op when those switches are used and the slot
> doesn't exist, over "no one will ever do that". Because someone will.
> I'm happy to hear suggestions on what to do in that case beyond exit
> cleanly.
Nonsensical option combinations should result in an error.
The more I think about it, I don't think it's nonsensical, though. --create-slot --if-exists or --drop-slot --if-not-exists, obviously. I mean, do you even logic?
Those aside, --if-exists just means a non-existent slot isn't an error condition, doesn't it? --start --if-exists will start, if the slot exists. Otherwise it won't, in neither case raising an error. Exactly what it says on the tin. Perhaps the docs could make clear that combination implies --no-loop (or at least means we'll exit immediately) if the slot does not, in fact, exist?
Because I started work on this patch in the context of doing some scripting around pg_recvlogical, I guess I had some vague notion that someone might have logic in their own scripts where that state was possible (and, of course, appropriately handled). The program exits either way: one assumes the operator meant to do that; the other says they were wrong to have done so.
I'm having trouble seeing a combination of options that does what it prima facie implies as an error state. If there's broader consensus that's how it should behave, I'll happily defer, though.
To that end, could I solicit some input from the broader audience?
It appears that you have removed the interaction of --start and
--if-exists in your last patch, but the documentation patch still
mentions it. Which is correct?
Pardon? I've had a bit on my plate recently, so it's entirely possible, if not likely, that I missed something, but isn't that this block?
@@ -267,6 +271,17 @@ StreamLogicalLog(void)
res = PQexec(conn, query->data);
if (PQresultStatus(res) != PGRES_COPY_BOTH)
{
+ const char* sqlstate = PQresultErrorField(res, PG_DIAG_SQLSTATE);
+
+ if (slot_not_exists_ok &&
+ sqlstate &&
+ strcmp(sqlstate, ERRCODE_UNKNOWN_OBJECT) == 0)
+ {
+ destroyPQExpBuffer(query);
+ PQclear(res);
+ disconnect_and_exit(0);
+ }
+
fprintf(stderr, _("%s: could not send replication command \"%s\": %s"),
progname, query->data, PQresultErrorMessage(res));
PQclear(res);
:wq
On 9/20/17 02:26, Rosser Schwarz wrote: > The more I think about it, I don't think it's nonsensical, though. > --create-slot --if-exists or --drop-slot --if-not-exists, obviously. I > mean, do you even logic? Those pieces make sense. We have many CREATE IF NOT EXISTS and DROP IF EXISTS commands. The use is clear. > Those aside, --if-exists just means a non-existent slot isn't an error > condition, doesn't it? --start --if-exists will start, if the slot > exists. Otherwise it won't, in neither case raising an error. Exactly > what it says on the tin. Perhaps the docs could make clear that > combination implies --no-loop (or at least means we'll exit immediately) > if the slot does not, in fact, exist? A non-terrible definition could perhaps be arrived at here, but it's not clear why one would need this. We don't have SELECT FROM IF EXISTS, either. I suggest you create a patch that focuses on the --create part. You can create a separate follow-on patch for the --start part if you wish, but I think that that patch would be rejected. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On 9/22/17 15:31, Peter Eisentraut wrote: > I suggest you create a patch that focuses on the --create part. > > You can create a separate follow-on patch for the --start part if you > wish, but I think that that patch would be rejected. I have moved this entry to the next commit fest, awaiting your updated patch. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 29, 2017 at 3:06 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 9/22/17 15:31, Peter Eisentraut wrote: >> I suggest you create a patch that focuses on the --create part. >> >> You can create a separate follow-on patch for the --start part if you >> wish, but I think that that patch would be rejected. > > I have moved this entry to the next commit fest, awaiting your updated > patch. I'm looking for simple patches to review so doing some gardening. Per Peter's message, moved this to Waiting on author. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Sep 29, 2017 at 10:06 PM, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> wrote: > On 9/22/17 15:31, Peter Eisentraut wrote: >> I suggest you create a patch that focuses on the --create part. >> >> You can create a separate follow-on patch for the --start part if you >> wish, but I think that that patch would be rejected. > > I have moved this entry to the next commit fest, awaiting your updated > patch. Still waiting for an update, and marked as returned with feedback. It seems to me that your patch adds a couple of option interactions, so this should be double-checked with exiting options and a set of TAP tests should be added as well. -- Michael
On 11/27/17 21:11, Michael Paquier wrote: > On Fri, Sep 29, 2017 at 10:06 PM, Peter Eisentraut > <peter.eisentraut@2ndquadrant.com> wrote: >> On 9/22/17 15:31, Peter Eisentraut wrote: >>> I suggest you create a patch that focuses on the --create part. >>> >>> You can create a separate follow-on patch for the --start part if you >>> wish, but I think that that patch would be rejected. >> >> I have moved this entry to the next commit fest, awaiting your updated >> patch. > > Still waiting for an update, and marked as returned with feedback. It > seems to me that your patch adds a couple of option interactions, so > this should be double-checked with exiting options and a set of TAP > tests should be added as well. It seems the author hasn't posted in two months, so maybe this patch is dead for now. -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services