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
Attachments:
pg_recvlogical--if-exists-v1.patchapplication/octet-stream; name=pg_recvlogical--if-exists-v1.patchDownload+54-4
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
--
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, 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!
https://commitfest.postgresql.org/14/1256/
--
: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!
<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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
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.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.
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>
wrote:
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
Attachments:
pg_recvlogical--if-exists-v2.patchapplication/octet-stream; name=pg_recvlogical--if-exists-v2.patchDownload+81-7
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