Thread: Re: [HACKERS] logical replication syntax (was DROP SUBSCRIPTION,query cancellations and slot handling)

On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> DROP SUBSCRIPTION mysub NODROP SLOT;

I'm pretty uninspired by this choice of syntax.  Logical replication
seems to have added a whole bunch of syntax that involves prefixing
words with "no".  In various places, there's NODROP, NOREFRESH, NOCOPY
DATA, NOCONNECT, and NOPUBLISH.  But "NO" is not an English prefix,
and there's no precedent of which I'm aware for such SQL syntax.  In
most places, we've chosen to name the option and then the user set it
to "on" or "off".  So for example you type EXPLAIN (ANALYZE, TIMING
OFF) or EXPLAIN (ANALYZE, TIMING FALSE), not EXPLAIN (ANALYZE,
NOTIMING).  I think most of the logical replication stuff could have
been done this way pretty easily, but for some reason it picked a
completely different approach.

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

On 2 May 2017 at 12:55, Robert Haas <robertmhaas@gmail.com> wrote:
> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> DROP SUBSCRIPTION mysub NODROP SLOT;
>
> I'm pretty uninspired by this choice of syntax.  Logical replication
> seems to have added a whole bunch of syntax that involves prefixing
> words with "no".  In various places, there's NODROP, NOREFRESH, NOCOPY
> DATA, NOCONNECT, and NOPUBLISH.  But "NO" is not an English prefix,
> and there's no precedent of which I'm aware for such SQL syntax.  In
> most places, we've chosen to name the option and then the user set it
> to "on" or "off".  So for example you type EXPLAIN (ANALYZE, TIMING
> OFF) or EXPLAIN (ANALYZE, TIMING FALSE), not EXPLAIN (ANALYZE,
> NOTIMING).  I think most of the logical replication stuff could have
> been done this way pretty easily, but for some reason it picked a
> completely different approach.

+1 for not upsetting my OCD.

Thom

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> DROP SUBSCRIPTION mysub NODROP SLOT;

> I'm pretty uninspired by this choice of syntax.  Logical replication
> seems to have added a whole bunch of syntax that involves prefixing
> words with "no".  In various places, there's NODROP, NOREFRESH, NOCOPY
> DATA, NOCONNECT, and NOPUBLISH.  But "NO" is not an English prefix,
> and there's no precedent of which I'm aware for such SQL syntax.  In
> most places, we've chosen to name the option and then the user set it
> to "on" or "off".  So for example you type EXPLAIN (ANALYZE, TIMING
> OFF) or EXPLAIN (ANALYZE, TIMING FALSE), not EXPLAIN (ANALYZE,
> NOTIMING).  I think most of the logical replication stuff could have
> been done this way pretty easily, but for some reason it picked a
> completely different approach.

I tend to agree with this criticism, but it's not quite true that we
don't do this anywhere else --- see CREATE USER for one example, where
we have monstrosities like NOBYPASSRLS.  Still, at least those are single
words without arguments.  "NODROP SLOT" is pretty ugly, not least
because if you want to claim CREATE USER as syntax precedent, you ought
to spell it NODROPSLOT.

Having said that, I doubt that anyone would argue that CREATE USER is
anything but legacy syntax, or that our more recent syntax designs aren't
better models to follow.

It's not quite too late to revisit the syntax of the log rep commands
... shall we add this as an open item?
        regards, tom lane

On 02/05/17 14:13, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> DROP SUBSCRIPTION mysub NODROP SLOT;
> 
>> I'm pretty uninspired by this choice of syntax.  Logical replication
>> seems to have added a whole bunch of syntax that involves prefixing
>> words with "no".  In various places, there's NODROP, NOREFRESH, NOCOPY
>> DATA, NOCONNECT, and NOPUBLISH.  But "NO" is not an English prefix,
>> and there's no precedent of which I'm aware for such SQL syntax.  In
>> most places, we've chosen to name the option and then the user set it
>> to "on" or "off".  So for example you type EXPLAIN (ANALYZE, TIMING
>> OFF) or EXPLAIN (ANALYZE, TIMING FALSE), not EXPLAIN (ANALYZE,
>> NOTIMING).  I think most of the logical replication stuff could have
>> been done this way pretty easily, but for some reason it picked a
>> completely different approach.
> 
> I tend to agree with this criticism, but it's not quite true that we
> don't do this anywhere else --- see CREATE USER for one example, where
> we have monstrosities like NOBYPASSRLS.  Still, at least those are single
> words without arguments.  "NODROP SLOT" is pretty ugly, not least
> because if you want to claim CREATE USER as syntax precedent, you ought
> to spell it NODROPSLOT.
> 
> Having said that, I doubt that anyone would argue that CREATE USER is
> anything but legacy syntax, or that our more recent syntax designs aren't
> better models to follow.
> 
> It's not quite too late to revisit the syntax of the log rep commands
> ... shall we add this as an open item?

I am happy to implement something different, it's quite trivial to
change. But I am not going to propose anything different as I can't
think of better syntax (if I could I would have done it). I don't like
the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
it also seems to not map very well to action (as opposed to output
option as it is in EXPLAIN). It might not be very close to SQL way but
that's because SQL way would be do not do any of those default actions
unless they are actually asked for (ie NODROP SLOT would be default and
DROP SLOT would be the option) but that's IMHO less user friendly.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> DROP SUBSCRIPTION mysub NODROP SLOT;

>> I'm pretty uninspired by this choice of syntax.

Actually, this command has got much worse problems than whether you like
the spelling of its option: it shouldn't have an option in the first
place.  I put it to you as an inviolable rule that no object DROP command
should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
If it does, then you don't know what to do when the object is recursed
to by a cascaded drop.

It's possible that we could allow exceptions to this rule for object types
that can never have any dependencies.  But a subscription doesn't qualify
--- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
to me like it's got a dependency on a database, too.  (BTW, if
subscriptions are per-database, why is pg_subscription a shared catalog?
There were some pretty schizophrenic decisions here.)

So ISTM we need to get rid of the above-depicted syntax.  We could instead
have what-to-do-with-the-slot as a property of the subscription,
established at CREATE SUBSCRIPTION and possibly changed later by ALTER
SUBSCRIPTION.  Not quite sure what to call the option, maybe something
based on the concept of the subscription "owning" the slot.

I think this is a must-fix issue, and will put it on the open items
list.
        regards, tom lane

On 02/05/17 15:10, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> DROP SUBSCRIPTION mysub NODROP SLOT;
> 
>>> I'm pretty uninspired by this choice of syntax.
> 
> Actually, this command has got much worse problems than whether you like
> the spelling of its option: it shouldn't have an option in the first
> place.  I put it to you as an inviolable rule that no object DROP command
> should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
> If it does, then you don't know what to do when the object is recursed
> to by a cascaded drop.
> 
> It's possible that we could allow exceptions to this rule for object types
> that can never have any dependencies.  But a subscription doesn't qualify
> --- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
> to me like it's got a dependency on a database, too.  (BTW, if
> subscriptions are per-database, why is pg_subscription a shared catalog?
> There were some pretty schizophrenic decisions here.)

Because otherwise we would need launcher process per database, not pretty.

> 
> So ISTM we need to get rid of the above-depicted syntax.  We could instead
> have what-to-do-with-the-slot as a property of the subscription,
> established at CREATE SUBSCRIPTION and possibly changed later by ALTER
> SUBSCRIPTION.  Not quite sure what to call the option, maybe something
> based on the concept of the subscription "owning" the slot.
> 

So what do you do if the upstream does not exist anymore when you are
dropping subscription?

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 02/05/17 15:14, Petr Jelinek wrote:
> On 02/05/17 15:10, Tom Lane wrote:
>> Robert Haas <robertmhaas@gmail.com> writes:
>>>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>>> DROP SUBSCRIPTION mysub NODROP SLOT;
>>
>>>> I'm pretty uninspired by this choice of syntax.
>>
>> Actually, this command has got much worse problems than whether you like
>> the spelling of its option: it shouldn't have an option in the first
>> place.  I put it to you as an inviolable rule that no object DROP command
>> should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
>> If it does, then you don't know what to do when the object is recursed
>> to by a cascaded drop.
>>
>> It's possible that we could allow exceptions to this rule for object types
>> that can never have any dependencies.  But a subscription doesn't qualify
>> --- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
>> to me like it's got a dependency on a database, too.  (BTW, if
>> subscriptions are per-database, why is pg_subscription a shared catalog?
>> There were some pretty schizophrenic decisions here.)
> 
> Because otherwise we would need launcher process per database, not pretty.
> 
>>
>> So ISTM we need to get rid of the above-depicted syntax.  We could instead
>> have what-to-do-with-the-slot as a property of the subscription,
>> established at CREATE SUBSCRIPTION and possibly changed later by ALTER
>> SUBSCRIPTION.  Not quite sure what to call the option, maybe something
>> based on the concept of the subscription "owning" the slot.
>>
> 
> So what do you do if the upstream does not exist anymore when you are
> dropping subscription?
> 

Let me expand, if we don't drop the slot by default when dropping
subscription, we'll have a lot of users with dead slots laying around
holding back WAL/catalog_xmin, that's really bad. If we do drop by
default like now, we need option to not do that, neither RESTRICT, nor
CASCADE fit that.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
> Let me expand, if we don't drop the slot by default when dropping
> subscription, we'll have a lot of users with dead slots laying around
> holding back WAL/catalog_xmin, that's really bad. If we do drop by
> default like now, we need option to not do that, neither RESTRICT, nor
> CASCADE fit that.

I'm not sure which part of "you can't have an option in DROP" isn't
clear to you.  Whatever the default behavior is always has to work,
because that is what's going to happen during DROP OWNED BY, or
DROP DATABASE.  If you want more than one behavior during DROP,
you need to drive that off something represented as a persistent
property of the object, not off an option in the DROP command.

I agree that RESTRICT/CASCADE don't fit this, unless the model
is that the slot is always owned by the subscription, which might
be too restrictive.
        regards, tom lane

On 05/02/2017 05:13 AM, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> DROP SUBSCRIPTION mysub NODROP SLOT;

> Having said that, I doubt that anyone would argue that CREATE USER is
> anything but legacy syntax, or that our more recent syntax designs aren't
> better models to follow.
>
> It's not quite too late to revisit the syntax of the log rep commands
> ... shall we add this as an open item?

I would think so. Just in reading this, even if we keep a similar syntax 
it should be DROP SLOT NO or DROP SLOT FALSE.

JD


>
>             regards, tom lane
>
>


-- 
Command Prompt, Inc.                  http://the.postgres.company/                        +1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.

On 02/05/17 15:31, Tom Lane wrote:
> Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
>> Let me expand, if we don't drop the slot by default when dropping
>> subscription, we'll have a lot of users with dead slots laying around
>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>> default like now, we need option to not do that, neither RESTRICT, nor
>> CASCADE fit that.
> 
> I'm not sure which part of "you can't have an option in DROP" isn't
> clear to you.  Whatever the default behavior is always has to work,
> because that is what's going to happen during DROP OWNED BY, or
> DROP DATABASE. 

You are saying it like there was some guarantee that those commands
can't fail because of other objects. AFAIK for example drop database can
trivially fail just because there is replication slot in it before PG10
(IIRC Craig managed to fix that in 10 for unrelated reasons).


> If you want more than one behavior during DROP,
> you need to drive that off something represented as a persistent
> property of the object, not off an option in the DROP command.
> 

I don't see how changing behavior as object property will change
anything in terms of not failing to cascade. If you use CREATE or ALTER
to say that subscription must drop slot and that fails then the cascade
will still fail so then you need to run ALTER again to change that
property. I fail to see how that's easier than running the DROP directly.

So the only way to fulfill the requirement you stated is to just not try
to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
behavior leave resources on upstream that will eventually cause that
server to stop unless user notices before. I think we better invent
something that limits how much inactive slots can hold back WAL and
catalog_xmin in this release as well then.

We should also not create the slot (at least by default) on CREATE
SUBSCRIPTION to have some symmetry.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Petr Jelinek wrote:

> So the only way to fulfill the requirement you stated is to just not try
> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
> behavior leave resources on upstream that will eventually cause that
> server to stop unless user notices before. I think we better invent
> something that limits how much inactive slots can hold back WAL and
> catalog_xmin in this release as well then.

I don't understand why isn't the default behavior to unconditionally
drop the slot.  Why do we ever want the slot to be kept?

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Petr Jelinek wrote:
>> So the only way to fulfill the requirement you stated is to just not try
>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
>> behavior leave resources on upstream that will eventually cause that
>> server to stop unless user notices before. I think we better invent
>> something that limits how much inactive slots can hold back WAL and
>> catalog_xmin in this release as well then.
>
> I don't understand why isn't the default behavior to unconditionally
> drop the slot.  Why do we ever want the slot to be kept?

What if the remote server doesn't exist any more?

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

On 02/05/17 18:00, Robert Haas wrote:
> On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
>> Petr Jelinek wrote:
>>> So the only way to fulfill the requirement you stated is to just not try
>>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
>>> behavior leave resources on upstream that will eventually cause that
>>> server to stop unless user notices before. I think we better invent
>>> something that limits how much inactive slots can hold back WAL and
>>> catalog_xmin in this release as well then.
>>
>> I don't understand why isn't the default behavior to unconditionally
>> drop the slot.  Why do we ever want the slot to be kept?
> 
> What if the remote server doesn't exist any more?
> 

Or what if the slot is used by other subscription (because you restored
dump containing subscriptions to another server for example), or you
have server that does not have outside network access anymore, or many
other reasons.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

Petr Jelinek wrote:
> On 02/05/17 18:00, Robert Haas wrote:
> > On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
> > <alvherre@2ndquadrant.com> wrote:
> >> Petr Jelinek wrote:
> >>> So the only way to fulfill the requirement you stated is to just not try
> >>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
> >>> behavior leave resources on upstream that will eventually cause that
> >>> server to stop unless user notices before. I think we better invent
> >>> something that limits how much inactive slots can hold back WAL and
> >>> catalog_xmin in this release as well then.
> >>
> >> I don't understand why isn't the default behavior to unconditionally
> >> drop the slot.  Why do we ever want the slot to be kept?
> > 
> > What if the remote server doesn't exist any more?
> 
> Or what if the slot is used by other subscription (because you restored
> dump containing subscriptions to another server for example), or you
> have server that does not have outside network access anymore, or many
> other reasons.

So there are two different scenarios: one is that you expect the slot
drop to fail for whatever reason; the other is that you want the slot to
be kept because it's needed for something else.  Maybe those two should
be considered separately.

1) keep the slot because it's needed for something else.  I see two options:  a) change the something else so that it
usesanother slot with the     same location.  Do we have some sort of "clone slot" operation?  b) have an option to
dissociatethe slot from the subscription prior     to the drop.
 

2) don't drop because we know it won't work.  I see two options:  c) ignore a drop slot failure, i.e. don't cause a
transactionabort.     An easy way to implement this is just add a PG_TRY block, but we     dislike adding those and not
re-throwingthe error.  d) rethink drop slot completely; maybe instead of doing it     immediately, it should be a
separatetask, so we first close the     current transaction (which dropped the subscription) and then we open     a
secondone to drop the slot, so that if the drop slot fails, the     subscription does not come back to life.
 

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, May 2, 2017 at 12:25 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> 2) don't drop because we know it won't work.  I see two options:
>    c) ignore a drop slot failure, i.e. don't cause a transaction abort.
>       An easy way to implement this is just add a PG_TRY block, but we
>       dislike adding those and not re-throwing the error.

Dislike doesn't seem like the right word.  Unless you rollback a
(sub)transaction, none of the cleanup that would normally do is done,
so you might leak buffer pins, memory, or other resources.  Unless the
code that can be run in the TRY/CATCH block is sufficiently restricted
as to make that a non-issue, which is rarely the case, it's not going
to work reliably at all.  I wonder why this API was even designed in a
way that made not re-throwing the error an option.

(I've wondered whether we should have some kind of mini-transaction
that is cheaper to abort but does only a critical subset of the
cleanup, but I haven't been able to figure out how you'd know whether
you only need to blow up the mini-transaction or whether you need to
kill the enclosing real (sub)transaction.)

>    d) rethink drop slot completely; maybe instead of doing it
>       immediately, it should be a separate task, so we first close the
>       current transaction (which dropped the subscription) and then we open
>       a second one to drop the slot, so that if the drop slot fails, the
>       subscription does not come back to life.

Something like this might work, although it's not clear how it
interacts with DROP .. CASCADE.  See
http://postgr.es/m/CA+Tgmob_hy0uQS9vq_9rDBgjpww3D3jBZ6twAKZOwaZigo4C3g@mail.gmail.com
for a very related point about adding subscriptions.

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

On Tue, May 2, 2017 at 8:45 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> I am happy to implement something different, it's quite trivial to
> change. But I am not going to propose anything different as I can't
> think of better syntax (if I could I would have done it). I don't like
> the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
> it also seems to not map very well to action (as opposed to output
> option as it is in EXPLAIN). It might not be very close to SQL way but
> that's because SQL way would be do not do any of those default actions
> unless they are actually asked for (ie NODROP SLOT would be default and
> DROP SLOT would be the option) but that's IMHO less user friendly.

So the cases where this "NO" prefixing comes up are:

1. CREATE SUBSCRIPTION

<phrase>where <replaceable class="PARAMETER">option</replaceable> can
be:</phrase>
   | ENABLED | DISABLED   | CREATE SLOT | NOCREATE SLOT   | SLOT NAME = <replaceable
class="PARAMETER">slot_name</replaceable>  | COPY DATA | NOCOPY DATA   | SYNCHRONOUS_COMMIT = <replaceable
 
class="PARAMETER">synchronous_commit</replaceable>   | NOCONNECT

I think it would have been a lot better to use the extensible options
syntax for this instead of inventing something new that's not even
consistent with itself. You've got SYNCHRONOUS_COMMIT with a hyphen
and CREATE SLOT with no hyphen and NOCOPY DATA with no hyphen and a
space left out. With the extensible options syntax, this would be
(enabled true/false, create_slot true/false, slot_name whatever,
synchronous_commit true/false, connect true/false). If we're going to
keep the present monstrosity, we can I think still change NOCONNECT to
NO CONNECT, but there's no fixing NOCOPY DATA in this syntax model.

2. ALTER SUBSCRIPTION

ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] {
REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }

There is no obvious reason why this could not have been spelled NO
REFRESH instead of adding a new keyword.

3. DROP SUBSCRIPTION

DROP SUBSCRIPTION [ IF EXISTS ] name [ DROP SLOT | NODROP SLOT ]

This is where we started, and I have nothing to add to what I (and
Tom) have already said.

4. CREATE PUBLICATION

CREATE PUBLICATION <replaceable class="parameter">name</replaceable>   [ FOR TABLE [ ONLY ] <replaceable
class="parameter">table_name</replaceable> [ * ] [, ...]     | FOR ALL TABLES ]   [ WITH ( <replaceable
class="parameter">option</replaceable>[, ... ] ) ]
 

<phrase>where <replaceable class="parameter">option</replaceable> can
be:</phrase>
     PUBLISH INSERT | NOPUBLISH INSERT   | PUBLISH UPDATE | NOPUBLISH UPDATE   | PUBLISH DELETE | NOPUBLISH DELETE

Again, the extensible options syntax like we use for EXPLAIN would
have been better here.  You could have said (publish_insert
true/false, publish_update true/false, publish_delete true/false), for
instance, or combined them into a single option like (publish
'insert,update') to omit deletes.

So it doesn't actually look hard to get rid of all of the NO prefixes.

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

On 02/05/17 19:42, Robert Haas wrote:
> On Tue, May 2, 2017 at 8:45 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> I am happy to implement something different, it's quite trivial to
>> change. But I am not going to propose anything different as I can't
>> think of better syntax (if I could I would have done it). I don't like
>> the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
>> it also seems to not map very well to action (as opposed to output
>> option as it is in EXPLAIN). It might not be very close to SQL way but
>> that's because SQL way would be do not do any of those default actions
>> unless they are actually asked for (ie NODROP SLOT would be default and
>> DROP SLOT would be the option) but that's IMHO less user friendly.
> 
> So the cases where this "NO" prefixing comes up are:
> 
> 1. CREATE SUBSCRIPTION
> 
> <phrase>where <replaceable class="PARAMETER">option</replaceable> can
> be:</phrase>
> 
>     | ENABLED | DISABLED
>     | CREATE SLOT | NOCREATE SLOT
>     | SLOT NAME = <replaceable class="PARAMETER">slot_name</replaceable>
>     | COPY DATA | NOCOPY DATA
>     | SYNCHRONOUS_COMMIT = <replaceable
> class="PARAMETER">synchronous_commit</replaceable>
>     | NOCONNECT
> 
> I think it would have been a lot better to use the extensible options
> syntax for this instead of inventing something new that's not even
> consistent with itself.

I am not sure what you mean by this, we always have to invent option
names if they are new options, even if we use generic options (which I
guess is what you mean by "extensible options syntax"). I used the
definitions instead of generic options, this means that the supported
syntax also includes COPY DATA = true/false, CREATE SLOT = true/false
etc, the NO* are just shorthands, it's quite simple to remove those.

> You've got SYNCHRONOUS_COMMIT with a hyphen
> and CREATE SLOT with no hyphen and NOCOPY DATA with no hyphen and a
> space left out. With the extensible options syntax, this would be
> (enabled true/false, create_slot true/false, slot_name whatever,
> synchronous_commit true/false, connect true/false). If we're going to
> keep the present monstrosity, we can I think still change NOCONNECT to
> NO CONNECT, but there's no fixing NOCOPY DATA in this syntax model.

See above.

> 
> 2. ALTER SUBSCRIPTION
> 
> ALTER SUBSCRIPTION name SET PUBLICATION publication_name [, ...] {
> REFRESH WITH ( puboption [, ... ] ) | NOREFRESH }
> 
> There is no obvious reason why this could not have been spelled NO
> REFRESH instead of adding a new keyword.
> 
> 3. DROP SUBSCRIPTION
> 
> DROP SUBSCRIPTION [ IF EXISTS ] name [ DROP SLOT | NODROP SLOT ]
> 
> This is where we started, and I have nothing to add to what I (and
> Tom) have already said.
> 
> 4. CREATE PUBLICATION
> 
> CREATE PUBLICATION <replaceable class="parameter">name</replaceable>
>     [ FOR TABLE [ ONLY ] <replaceable
> class="parameter">table_name</replaceable> [ * ] [, ...]
>       | FOR ALL TABLES ]
>     [ WITH ( <replaceable class="parameter">option</replaceable> [, ... ] ) ]
> 
> <phrase>where <replaceable class="parameter">option</replaceable> can
> be:</phrase>
> 
>       PUBLISH INSERT | NOPUBLISH INSERT
>     | PUBLISH UPDATE | NOPUBLISH UPDATE
>     | PUBLISH DELETE | NOPUBLISH DELETE
> 
> Again, the extensible options syntax like we use for EXPLAIN would
> have been better here.  You could have said (publish_insert
> true/false, publish_update true/false, publish_delete true/false), for
> instance, or combined them into a single option like (publish
> 'insert,update') to omit deletes.
> 
> So it doesn't actually look hard to get rid of all of the NO prefixes.
> 

That sounds okay. I know PeterE didn't like the lower case and
underscore separated words for options in the original patch, so I'd
like to hear his opinion on this. I am not sure how much advantage is
there in removing the '=' in between the key and value. That's the main
difference between generic options and definitions (well and definitions
can have 2 words for key, but that's something I have added anyway), I
don't really understand why we have both and use one for some commends
and the other for others btw.


--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, May 2, 2017 at 3:02 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> That sounds okay. I know PeterE didn't like the lower case and
> underscore separated words for options in the original patch, so I'd
> like to hear his opinion on this. I am not sure how much advantage is
> there in removing the '=' in between the key and value. That's the main
> difference between generic options and definitions (well and definitions
> can have 2 words for key, but that's something I have added anyway), I
> don't really understand why we have both and use one for some commends
> and the other for others btw.

I don't care all *that* much about the equals sign, although I think
it's marginally more elegant without it, and VACUUM, EXPLAIN, and COPY
all do it that way.  But I think allowing two-word names is just a
pile of parser nastiness that we'd be far better off without.  If you
use the same syntax as VACUUM, EXPLAIN, and COPY, all options are a
single identifier.  If it's got to be multiple words, they can be
separated by underscores.  But with what you've got right now, it's
sometimes one identifier even when it's two words, and sometimes two
identifiers.  When it's three words, it's two identifiers, with two of
them concatenated.

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

Robert Haas wrote:
> On Tue, May 2, 2017 at 12:25 PM, Alvaro Herrera
> <alvherre@2ndquadrant.com> wrote:
> > 2) don't drop because we know it won't work.  I see two options:
> >    c) ignore a drop slot failure, i.e. don't cause a transaction abort.
> >       An easy way to implement this is just add a PG_TRY block, but we
> >       dislike adding those and not re-throwing the error.
> 
> Dislike doesn't seem like the right word.  Unless you rollback a
> (sub)transaction, none of the cleanup that would normally do is done,

True.  So one possible implementation is that we open a subtransaction
before dropping the slot, and we abort it if things go south.  This is a
bit slower, but not critically so.

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On Tue, May 2, 2017 at 5:15 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Tue, May 2, 2017 at 12:25 PM, Alvaro Herrera
>> <alvherre@2ndquadrant.com> wrote:
>> > 2) don't drop because we know it won't work.  I see two options:
>> >    c) ignore a drop slot failure, i.e. don't cause a transaction abort.
>> >       An easy way to implement this is just add a PG_TRY block, but we
>> >       dislike adding those and not re-throwing the error.
>>
>> Dislike doesn't seem like the right word.  Unless you rollback a
>> (sub)transaction, none of the cleanup that would normally do is done,
>
> True.  So one possible implementation is that we open a subtransaction
> before dropping the slot, and we abort it if things go south.  This is a
> bit slower, but not critically so.

I think that could work.  Subtransaction abort isn't as fast as I
would sometimes like, but for a DDL command the overhead is pretty
insignificant.

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

On 02/05/17 22:40, Robert Haas wrote:
> On Tue, May 2, 2017 at 3:02 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> That sounds okay. I know PeterE didn't like the lower case and
>> underscore separated words for options in the original patch, so I'd
>> like to hear his opinion on this. I am not sure how much advantage is
>> there in removing the '=' in between the key and value. That's the main
>> difference between generic options and definitions (well and definitions
>> can have 2 words for key, but that's something I have added anyway), I
>> don't really understand why we have both and use one for some commends
>> and the other for others btw.
> 
> I don't care all *that* much about the equals sign, although I think
> it's marginally more elegant without it, and VACUUM, EXPLAIN, and COPY
> all do it that way.  But I think allowing two-word names is just a
> pile of parser nastiness that we'd be far better off without.  If you
> use the same syntax as VACUUM, EXPLAIN, and COPY, all options are a
> single identifier.

Ok, Let me be clear, I actually happen to agree with your proposal. The
reason I am moaning is that I always seem to find myself doing tons of
mechanical work to rewrite some cosmetic aspect of some patch based on
which committer is paying attention in a specific week. So while I am
for doing exactly what you proposed, I'd like to see couple of +1s first
(Peter?) since I don't want to rewrite it to something different again
and it's also long past freeze.

To repeat the proposal:
- change the WITH (...) clauses in subscription/publication commands to:
(create_slot true/false, connect true/false, slot_name 'something',
copy_data true/false, etc)

- change the NOREFRESH to NO REFRESH in ALTER SUBSCRIPTION name SET
PUBLICATION (btw I originally had SKIP REFRESH there but changed it to
NOREFRESH for consistency with the other NO* stuff, wonder if SKIP would
sound more english).

- change the (publish insert/nopublish insert/publish update/nopublish
update), etc options to (publish 'update,insert').

And one question, if we are not using the definitions (key = value)
should we keep the WITH keyword in the syntax, would it be confusing?

>  If it's got to be multiple words, they can be
> separated by underscores.  But with what you've got right now, it's
> sometimes one identifier even when it's two words, and sometimes two
> identifiers. 
> 

The main inconsistency is the synchronous_commit which is that way to
make it clear it's same as the GUC it changes, but looks like that was a
mistake.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 02/05/17 18:25, Alvaro Herrera wrote:
> Petr Jelinek wrote:
>> On 02/05/17 18:00, Robert Haas wrote:
>>> On Tue, May 2, 2017 at 11:49 AM, Alvaro Herrera
>>> <alvherre@2ndquadrant.com> wrote:
>>>> Petr Jelinek wrote:
>>>>> So the only way to fulfill the requirement you stated is to just not try
>>>>> to drop the slot, ever, on DROP SUBSCRIPTION. That makes the default
>>>>> behavior leave resources on upstream that will eventually cause that
>>>>> server to stop unless user notices before. I think we better invent
>>>>> something that limits how much inactive slots can hold back WAL and
>>>>> catalog_xmin in this release as well then.
>>>>
>>>> I don't understand why isn't the default behavior to unconditionally
>>>> drop the slot.  Why do we ever want the slot to be kept?
>>>
>>> What if the remote server doesn't exist any more?
>>
>> Or what if the slot is used by other subscription (because you restored
>> dump containing subscriptions to another server for example), or you
>> have server that does not have outside network access anymore, or many
>> other reasons.
> 
> So there are two different scenarios: one is that you expect the slot
> drop to fail for whatever reason; the other is that you want the slot to
> be kept because it's needed for something else.  Maybe those two should
> be considered separately.
> 
> 1) keep the slot because it's needed for something else.
>    I see two options:
>    a) change the something else so that it uses another slot with the
>       same location.  Do we have some sort of "clone slot" operation?

Nope.

>    b) have an option to dissociate the slot from the subscription prior
>       to the drop.
> 

We do have ALTER SUBSCRIPTION bla WITH (SLOT NAME = 'something') so this
is definitely doable but ALTER SUBSCRIPTION bla WITH (SLOT NAME = '') is
not very nice syntax, maybe something like RESET SLOT NAME?

> 2) don't drop because we know it won't work.  I see two options:
>    c) ignore a drop slot failure, i.e. don't cause a transaction abort.
>       An easy way to implement this is just add a PG_TRY block, but we
>       dislike adding those and not re-throwing the error.
>    d) rethink drop slot completely; maybe instead of doing it
>       immediately, it should be a separate task, so we first close the
>       current transaction (which dropped the subscription) and then we open
>       a second one to drop the slot, so that if the drop slot fails, the
>       subscription does not come back to life.
> 

Yeah I was thinking about ignoring failures with WARNING as well, but
never seemed right because it would not solve the case 1), but I didn't
think of b) which might solve it.

I was also thinking on how to map the behavior to RESTRICT vs CASCADE,
wonder if RESTRICT should check for slot existence and refuse to drop if
the slot exists (question is then should it bail on connection failure
or ignore it?) and CASCADE should try to drop the slot and only warn on
failure then.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 02/05/17 16:14, Petr Jelinek wrote:
> On 02/05/17 15:31, Tom Lane wrote:
>> Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
>>> Let me expand, if we don't drop the slot by default when dropping
>>> subscription, we'll have a lot of users with dead slots laying around
>>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>>> default like now, we need option to not do that, neither RESTRICT, nor
>>> CASCADE fit that.
>>
>> I'm not sure which part of "you can't have an option in DROP" isn't
>> clear to you.  Whatever the default behavior is always has to work,
>> because that is what's going to happen during DROP OWNED BY, or
>> DROP DATABASE. 
> 
> You are saying it like there was some guarantee that those commands
> can't fail because of other objects. AFAIK for example drop database can
> trivially fail just because there is replication slot in it before PG10
> (IIRC Craig managed to fix that in 10 for unrelated reasons).
> 

Btw looks like I already forgot how this stuff behaves. Existence of
subscription currently also prevents DROP DATABASE (for same reason
active backends do).

DROP OWNED BY ignores SUBSCRIPTION too now, although I think it might
not be strictly necessary to stay that way.

But if we keep this behavior then the point about these two commands
cascading to subscriptions is largely moot.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> Ok, Let me be clear, I actually happen to agree with your proposal. The
> reason I am moaning is that I always seem to find myself doing tons of
> mechanical work to rewrite some cosmetic aspect of some patch based on
> which committer is paying attention in a specific week. So while I am
> for doing exactly what you proposed, I'd like to see couple of +1s first
> (Peter?) since I don't want to rewrite it to something different again
> and it's also long past freeze.

So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
favor of cleaning this up.  Perhaps they could opine on this
particular proposal.

> To repeat the proposal:
> - change the WITH (...) clauses in subscription/publication commands to:
> (create_slot true/false, connect true/false, slot_name 'something',
> copy_data true/false, etc)
>
> - change the NOREFRESH to NO REFRESH in ALTER SUBSCRIPTION name SET
> PUBLICATION (btw I originally had SKIP REFRESH there but changed it to
> NOREFRESH for consistency with the other NO* stuff, wonder if SKIP would
> sound more english).
>
> - change the (publish insert/nopublish insert/publish update/nopublish
> update), etc options to (publish 'update,insert').
>
> And one question, if we are not using the definitions (key = value)
> should we keep the WITH keyword in the syntax, would it be confusing?

No opinion on that.

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

Robert Haas <robertmhaas@gmail.com> writes:
> On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> Ok, Let me be clear, I actually happen to agree with your proposal. The
>> reason I am moaning is that I always seem to find myself doing tons of
>> mechanical work to rewrite some cosmetic aspect of some patch based on
>> which committer is paying attention in a specific week. So while I am
>> for doing exactly what you proposed, I'd like to see couple of +1s first
>> (Peter?) since I don't want to rewrite it to something different again
>> and it's also long past freeze.

> So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
> favor of cleaning this up.  Perhaps they could opine on this
> particular proposal.

It seems like there's some remaining indecision between "make it look
like the options in EXPLAIN, VACUUM, etc" and "make it look like the
WITH options found in some other statements".  I do not have a strong
opinion which one to do, but I'd definitely say that you should use WITH
in the latter case but not in the former.  I think this mostly boils down
to whether to use "=" or not; you've got "not" in the proposal, which
means you are following the EXPLAIN precedent and should not use WITH.

I'm okay with the other specifics mentioned.
        regards, tom lane

On 04/05/17 23:29, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Wed, May 3, 2017 at 12:38 AM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> Ok, Let me be clear, I actually happen to agree with your proposal. The
>>> reason I am moaning is that I always seem to find myself doing tons of
>>> mechanical work to rewrite some cosmetic aspect of some patch based on
>>> which committer is paying attention in a specific week. So while I am
>>> for doing exactly what you proposed, I'd like to see couple of +1s first
>>> (Peter?) since I don't want to rewrite it to something different again
>>> and it's also long past freeze.
> 
>> So, Tom Lane and Thom Brown and Josh Drake all seemed generally in
>> favor of cleaning this up.  Perhaps they could opine on this
>> particular proposal.
> 
> It seems like there's some remaining indecision between "make it look
> like the options in EXPLAIN, VACUUM, etc" and "make it look like the
> WITH options found in some other statements".  I do not have a strong
> opinion which one to do, but I'd definitely say that you should use WITH
> in the latter case but not in the former.  I think this mostly boils down
> to whether to use "=" or not; you've got "not" in the proposal, which
> means you are following the EXPLAIN precedent and should not use WITH.
> 

Okay, here is my initial attempt on changing this. I opted for WITH and
"=" as I like it slightly better (also the generic_options expect values
to be quoted which I don't like and then I would have to again invent
something like generic_options but not quite the same).

Most of the changes go to doc and tests, not that much code has changed
as I already used the definiton (which is the parser's name for these
WITH things). Except that I removed the NO shorthands and changed
publish_insert,etc to publish = 'insert,...'. I also changed the
NOREFRESH to SKIP REFRESH.

I didn't touch the DROP SUBSCRIPTION slot handling so far, that needs to
be separate patch as there is behavior change there, while this is
purely cosmetic and IMHO it's better to not mix those. (I plan to send
patch for that which changes the behavior heavily soonish as well)

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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 02/05/17 15:31, Tom Lane wrote:
> Petr Jelinek <petr.jelinek@2ndquadrant.com> writes:
>> Let me expand, if we don't drop the slot by default when dropping
>> subscription, we'll have a lot of users with dead slots laying around
>> holding back WAL/catalog_xmin, that's really bad. If we do drop by
>> default like now, we need option to not do that, neither RESTRICT, nor
>> CASCADE fit that.
> 
> I'm not sure which part of "you can't have an option in DROP" isn't
> clear to you.  Whatever the default behavior is always has to work,
> because that is what's going to happen during DROP OWNED BY, or
> DROP DATABASE.  If you want more than one behavior during DROP,
> you need to drive that off something represented as a persistent
> property of the object, not off an option in the DROP command.
> 
> I agree that RESTRICT/CASCADE don't fit this, unless the model
> is that the slot is always owned by the subscription, which might
> be too restrictive.
> 

What do you think of attached. I actually did add RESTRICT/CASCADE, in a
way that if there is slot RESTRICT will refuse to drop subscription and
CASCADE will try to drop it. Still all errors.

The new way to not drop slot is to set slot_name to NONE which is new
value that I invented (inspiration from OWNED BY sequences) which
basically disassociates the subscription from slot.

It's slightly less automatic this way but perhaps that's not a bad
thing, at least nobody will drop slots by mistake while we still make
sure that slots are not left over without anybody noticing.

Note that this would need catalog version bump as it removes NOT NULL
constraint from subslotname.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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, May 02, 2017 at 09:10:52AM -0400, Tom Lane wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
> >> On Thu, Apr 20, 2017 at 7:46 AM, Petr Jelinek
> >> <petr.jelinek@2ndquadrant.com> wrote:
> >>> DROP SUBSCRIPTION mysub NODROP SLOT;
> 
> >> I'm pretty uninspired by this choice of syntax.
> 
> Actually, this command has got much worse problems than whether you like
> the spelling of its option: it shouldn't have an option in the first
> place.  I put it to you as an inviolable rule that no object DROP command
> should ever have any options other than RESTRICT/CASCADE and IF EXISTS.
> If it does, then you don't know what to do when the object is recursed
> to by a cascaded drop.
> 
> It's possible that we could allow exceptions to this rule for object types
> that can never have any dependencies.  But a subscription doesn't qualify
> --- it's got an owner, so DROP OWNED BY already poses this problem.  Looks
> to me like it's got a dependency on a database, too.  (BTW, if
> subscriptions are per-database, why is pg_subscription a shared catalog?
> There were some pretty schizophrenic decisions here.)
> 
> So ISTM we need to get rid of the above-depicted syntax.  We could instead
> have what-to-do-with-the-slot as a property of the subscription,
> established at CREATE SUBSCRIPTION and possibly changed later by ALTER
> SUBSCRIPTION.  Not quite sure what to call the option, maybe something
> based on the concept of the subscription "owning" the slot.
> 
> I think this is a must-fix issue, and will put it on the open items
> list.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On Tue, May 02, 2017 at 01:42:37PM -0400, Robert Haas wrote:
> On Tue, May 2, 2017 at 8:45 AM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
> > I am happy to implement something different, it's quite trivial to
> > change. But I am not going to propose anything different as I can't
> > think of better syntax (if I could I would have done it). I don't like
> > the OFF or FALSE (ie DROP SLOT OFF) any more than what is there now and
> > it also seems to not map very well to action (as opposed to output
> > option as it is in EXPLAIN). It might not be very close to SQL way but
> > that's because SQL way would be do not do any of those default actions
> > unless they are actually asked for (ie NODROP SLOT would be default and
> > DROP SLOT would be the option) but that's IMHO less user friendly.
> 
> So the cases where this "NO" prefixing comes up are:
> 
> 1. CREATE SUBSCRIPTION
...
> 2. ALTER SUBSCRIPTION
...
> 3. DROP SUBSCRIPTION
...
> 4. CREATE PUBLICATION
...
> So it doesn't actually look hard to get rid of all of the NO prefixes.

[Action required within three days.  This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item.  Peter,
since you committed the patch believed to have created it, you own this open
item.  If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know.  Otherwise, please observe the policy on
open item ownership[1] and send a status update within three calendar days of
this message.  Include a date for your subsequent status update.  Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10.  Consequently, I will appreciate your efforts
toward speedy resolution.  Thanks.

[1] https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 5/5/17 13:01, Petr Jelinek wrote:
> What do you think of attached. I actually did add RESTRICT/CASCADE, in a
> way that if there is slot RESTRICT will refuse to drop subscription and
> CASCADE will try to drop it. Still all errors.
> 
> The new way to not drop slot is to set slot_name to NONE which is new
> value that I invented (inspiration from OWNED BY sequences) which
> basically disassociates the subscription from slot.
> 
> It's slightly less automatic this way but perhaps that's not a bad
> thing, at least nobody will drop slots by mistake while we still make
> sure that slots are not left over without anybody noticing.

I think this is OK.  Could you send a version of this patch based on
master please?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/7/17 04:21, Noah Misch wrote:
> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> since you committed the patch believed to have created it, you own this open
> item.  If some other commit is more relevant or if this does not belong as a
> v10 open item, please let us know.  Otherwise, please observe the policy on
> open item ownership[1] and send a status update within three calendar days of
> this message.  Include a date for your subsequent status update.  Testers may
> discover new open items at any time, and I want to plan to get them all fixed
> well in advance of shipping v10.  Consequently, I will appreciate your efforts
> toward speedy resolution.  Thanks.

I think we have a workable patch, but it needs some rebasing.  I hope to
have this sorted by Wednesday.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 08/05/17 22:55, Peter Eisentraut wrote:
> On 5/5/17 13:01, Petr Jelinek wrote:
>> What do you think of attached. I actually did add RESTRICT/CASCADE, in a
>> way that if there is slot RESTRICT will refuse to drop subscription and
>> CASCADE will try to drop it. Still all errors.
>>
>> The new way to not drop slot is to set slot_name to NONE which is new
>> value that I invented (inspiration from OWNED BY sequences) which
>> basically disassociates the subscription from slot.
>>
>> It's slightly less automatic this way but perhaps that's not a bad
>> thing, at least nobody will drop slots by mistake while we still make
>> sure that slots are not left over without anybody noticing.
> 
> I think this is OK.  Could you send a version of this patch based on
> master please?
> 

Here it is.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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 5/8/17 17:55, Petr Jelinek wrote:
> On 08/05/17 22:55, Peter Eisentraut wrote:
>> On 5/5/17 13:01, Petr Jelinek wrote:
>>> What do you think of attached. I actually did add RESTRICT/CASCADE, in a
>>> way that if there is slot RESTRICT will refuse to drop subscription and
>>> CASCADE will try to drop it. Still all errors.
>>>
>>> The new way to not drop slot is to set slot_name to NONE which is new
>>> value that I invented (inspiration from OWNED BY sequences) which
>>> basically disassociates the subscription from slot.
>>>
>>> It's slightly less automatic this way but perhaps that's not a bad
>>> thing, at least nobody will drop slots by mistake while we still make
>>> sure that slots are not left over without anybody noticing.
>>
>> I think this is OK.  Could you send a version of this patch based on
>> master please?
>>
> 
> Here it is.

The way this uses RESTRICT and CASCADE appears to be backwards from its
usual meaning.  Normally, CASCADE when dropping an object that is still
used by others will cause those other objects to be dropped.  The
equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
subscription.

What we want to simulate instead is an "auto" dependency of the slot on
the subscription.  So you can drop the slot separately (subject to other
restrictions), and it is dropped automatically when the subscription is
dropped.  To avoid that, you can disassociate the slot from the
subscription, which you have implemented.

I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
associated with the subscription, it should be there when we drop the
subscription.  Otherwise, the user has to disassociate the slot and take
care of it manually.  So just keep the "cascade" behavior.

Similarly, I wouldn't check first whether the slot exists.  If the
subscription is associated with the slot, it should be there.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/8/17 23:23, Peter Eisentraut wrote:
> The way this uses RESTRICT and CASCADE appears to be backwards from its
> usual meaning.  Normally, CASCADE when dropping an object that is still
> used by others will cause those other objects to be dropped.  The
> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
> subscription.
> 
> What we want to simulate instead is an "auto" dependency of the slot on
> the subscription.  So you can drop the slot separately (subject to other
> restrictions), and it is dropped automatically when the subscription is
> dropped.  To avoid that, you can disassociate the slot from the
> subscription, which you have implemented.
> 
> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
> associated with the subscription, it should be there when we drop the
> subscription.  Otherwise, the user has to disassociate the slot and take
> care of it manually.  So just keep the "cascade" behavior.
> 
> Similarly, I wouldn't check first whether the slot exists.  If the
> subscription is associated with the slot, it should be there.

Here is your patch amended for that.

-- 
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, May 9, 2017 at 2:07 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/8/17 23:23, Peter Eisentraut wrote:
>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>> usual meaning.  Normally, CASCADE when dropping an object that is still
>> used by others will cause those other objects to be dropped.  The
>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>> subscription.
>>
>> What we want to simulate instead is an "auto" dependency of the slot on
>> the subscription.  So you can drop the slot separately (subject to other
>> restrictions), and it is dropped automatically when the subscription is
>> dropped.  To avoid that, you can disassociate the slot from the
>> subscription, which you have implemented.
>>
>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>> associated with the subscription, it should be there when we drop the
>> subscription.  Otherwise, the user has to disassociate the slot and take
>> care of it manually.  So just keep the "cascade" behavior.
>>
>> Similarly, I wouldn't check first whether the slot exists.  If the
>> subscription is associated with the slot, it should be there.
>
> Here is your patch amended for that.
>

I think we should change tab-completion support for that as well.

diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
index 183fc37..046fdd5 100644
--- a/src/bin/psql/tab-complete.c
+++ b/src/bin/psql/tab-complete.c
@@ -2684,7 +2684,7 @@ psql_completion(const char *text, int start, int end)
       /* DROP SUBSCRIPTION */       else if (Matches3("DROP", "SUBSCRIPTION", MatchAny))
-               COMPLETE_WITH_LIST2("DROP SLOT", "NODROP SLOT");
+               COMPLETE_WITH_LIST2("CASCADE", "RESTRICT");
/* EXECUTE */       else if (Matches1("EXECUTE"))

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 09/05/17 07:07, Peter Eisentraut wrote:
> On 5/8/17 23:23, Peter Eisentraut wrote:
>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>> usual meaning.  Normally, CASCADE when dropping an object that is still
>> used by others will cause those other objects to be dropped.  The
>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>> subscription.
>>
>> What we want to simulate instead is an "auto" dependency of the slot on
>> the subscription.  So you can drop the slot separately (subject to other
>> restrictions), and it is dropped automatically when the subscription is
>> dropped.  To avoid that, you can disassociate the slot from the
>> subscription, which you have implemented.
>>
>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>> associated with the subscription, it should be there when we drop the
>> subscription.  Otherwise, the user has to disassociate the slot and take
>> care of it manually.  So just keep the "cascade" behavior.
>>
>> Similarly, I wouldn't check first whether the slot exists.  If the
>> subscription is associated with the slot, it should be there.
> 
> Here is your patch amended for that.
> 

I am fine with this mechanism as well.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 09/05/17 07:07, Peter Eisentraut wrote:
>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>> used by others will cause those other objects to be dropped.  The
>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>> subscription.
>>>
>>> What we want to simulate instead is an "auto" dependency of the slot on
>>> the subscription.  So you can drop the slot separately (subject to other
>>> restrictions), and it is dropped automatically when the subscription is
>>> dropped.  To avoid that, you can disassociate the slot from the
>>> subscription, which you have implemented.
>>>
>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>> associated with the subscription, it should be there when we drop the
>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>> care of it manually.  So just keep the "cascade" behavior.
>>>
>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>> subscription is associated with the slot, it should be there.

IIUC in this design, for example if we mistakenly create a
subscription without creating replication slot and corresponding
replication slot doesn't exist on publisher, we cannot drop
subscription until we create corresponding replication slot manually.
Isn't it become a problem for user?

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 09/05/17 10:51, Masahiko Sawada wrote:
> On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 09/05/17 07:07, Peter Eisentraut wrote:
>>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>>> used by others will cause those other objects to be dropped.  The
>>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>>> subscription.
>>>>
>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>> the subscription.  So you can drop the slot separately (subject to other
>>>> restrictions), and it is dropped automatically when the subscription is
>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>> subscription, which you have implemented.
>>>>
>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>> associated with the subscription, it should be there when we drop the
>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>
>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>> subscription is associated with the slot, it should be there.
> 
> IIUC in this design, for example if we mistakenly create a
> subscription without creating replication slot and corresponding
> replication slot doesn't exist on publisher, we cannot drop
> subscription until we create corresponding replication slot manually.
> Isn't it become a problem for user?
> 

We can, that's why the NONE value for slot name was added by the patch
so that subscription can be made "slot-less". The change of
RESTRICT/CASCADE behavior that Peter made is just about whether we
refuse to drop subscription by default when there is slot associated
with or if we just go straight to dropping the slot.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On Tue, May 9, 2017 at 5:57 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:
> On 09/05/17 10:51, Masahiko Sawada wrote:
>> On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
>> <petr.jelinek@2ndquadrant.com> wrote:
>>> On 09/05/17 07:07, Peter Eisentraut wrote:
>>>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>>>> used by others will cause those other objects to be dropped.  The
>>>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>>>> subscription.
>>>>>
>>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>>> the subscription.  So you can drop the slot separately (subject to other
>>>>> restrictions), and it is dropped automatically when the subscription is
>>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>>> subscription, which you have implemented.
>>>>>
>>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>>> associated with the subscription, it should be there when we drop the
>>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>>
>>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>>> subscription is associated with the slot, it should be there.
>>
>> IIUC in this design, for example if we mistakenly create a
>> subscription without creating replication slot and corresponding
>> replication slot doesn't exist on publisher, we cannot drop
>> subscription until we create corresponding replication slot manually.
>> Isn't it become a problem for user?
>>
>
> We can, that's why the NONE value for slot name was added by the patch
> so that subscription can be made "slot-less".

Yeah, but since we can still create subscription with only NOCREATE
SLOT option (option name will be changed but still exists), if we do
that then non-NULL value is stored into subslotname and the
subscription is enable. We can drop such subscription after disabled
it and after set its slot name to NONE. But I think it's still not
good for user..

> The change of
> RESTRICT/CASCADE behavior that Peter made is just about whether we
> refuse to drop subscription by default when there is slot associated
> with or if we just go straight to dropping the slot.
>

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

On 09/05/17 11:44, Masahiko Sawada wrote:
> On Tue, May 9, 2017 at 5:57 PM, Petr Jelinek
> <petr.jelinek@2ndquadrant.com> wrote:
>> On 09/05/17 10:51, Masahiko Sawada wrote:
>>> On Tue, May 9, 2017 at 5:39 PM, Petr Jelinek
>>> <petr.jelinek@2ndquadrant.com> wrote:
>>>> On 09/05/17 07:07, Peter Eisentraut wrote:
>>>>> On 5/8/17 23:23, Peter Eisentraut wrote:
>>>>>> The way this uses RESTRICT and CASCADE appears to be backwards from its
>>>>>> usual meaning.  Normally, CASCADE when dropping an object that is still
>>>>>> used by others will cause those other objects to be dropped.  The
>>>>>> equivalent here would be DROP REPLICATION SLOT + CASCADE would drop the
>>>>>> subscription.
>>>>>>
>>>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>>>> the subscription.  So you can drop the slot separately (subject to other
>>>>>> restrictions), and it is dropped automatically when the subscription is
>>>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>>>> subscription, which you have implemented.
>>>>>>
>>>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>>>> associated with the subscription, it should be there when we drop the
>>>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>>>
>>>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>>>> subscription is associated with the slot, it should be there.
>>>
>>> IIUC in this design, for example if we mistakenly create a
>>> subscription without creating replication slot and corresponding
>>> replication slot doesn't exist on publisher, we cannot drop
>>> subscription until we create corresponding replication slot manually.
>>> Isn't it become a problem for user?
>>>
>>
>> We can, that's why the NONE value for slot name was added by the patch
>> so that subscription can be made "slot-less".
> 
> Yeah, but since we can still create subscription with only NOCREATE
> SLOT option (option name will be changed but still exists), if we do
> that then non-NULL value is stored into subslotname and the
> subscription is enable. We can drop such subscription after disabled
> it and after set its slot name to NONE. But I think it's still not
> good for user..
> 

Well that's why I originally had the options directly as part of DROP
SUBSCRIPTION. But if you read the discussion in this thread, that's not
something we should be doing. There is no sensible way of mapping the
nodrop behavior to the only allowed options of RESTRICT and CASCADE so
there is not much we can do other than the ALTER.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 5/9/17 04:39, Petr Jelinek wrote:
>>> What we want to simulate instead is an "auto" dependency of the slot on
>>> the subscription.  So you can drop the slot separately (subject to other
>>> restrictions), and it is dropped automatically when the subscription is
>>> dropped.  To avoid that, you can disassociate the slot from the
>>> subscription, which you have implemented.
>>>
>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>> associated with the subscription, it should be there when we drop the
>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>> care of it manually.  So just keep the "cascade" behavior.
>>>
>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>> subscription is associated with the slot, it should be there.
>>
>> Here is your patch amended for that.
> 
> I am fine with this mechanism as well.

Committed.

I also wrote a bit of documentation about slot handling for
subscriptions, covering some of what was discussed in this thread.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 5/9/17 03:27, Masahiko Sawada wrote:
> I think we should change tab-completion support for that as well.
> 
> diff --git a/src/bin/psql/tab-complete.c b/src/bin/psql/tab-complete.c
> index 183fc37..046fdd5 100644
> --- a/src/bin/psql/tab-complete.c
> +++ b/src/bin/psql/tab-complete.c
> @@ -2684,7 +2684,7 @@ psql_completion(const char *text, int start, int end)
> 
>         /* DROP SUBSCRIPTION */
>         else if (Matches3("DROP", "SUBSCRIPTION", MatchAny))
> -               COMPLETE_WITH_LIST2("DROP SLOT", "NODROP SLOT");
> +               COMPLETE_WITH_LIST2("CASCADE", "RESTRICT");
> 
>  /* EXECUTE */
>         else if (Matches1("EXECUTE"))

Thanks for pointing that out.  I just moved it under the generic DROP
support section.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 09/05/17 16:28, Peter Eisentraut wrote:
> On 5/9/17 04:39, Petr Jelinek wrote:
>>>> What we want to simulate instead is an "auto" dependency of the slot on
>>>> the subscription.  So you can drop the slot separately (subject to other
>>>> restrictions), and it is dropped automatically when the subscription is
>>>> dropped.  To avoid that, you can disassociate the slot from the
>>>> subscription, which you have implemented.
>>>>
>>>> I think we can therefore do without RESTRICT/CASCADE here.  If a slot is
>>>> associated with the subscription, it should be there when we drop the
>>>> subscription.  Otherwise, the user has to disassociate the slot and take
>>>> care of it manually.  So just keep the "cascade" behavior.
>>>>
>>>> Similarly, I wouldn't check first whether the slot exists.  If the
>>>> subscription is associated with the slot, it should be there.
>>>
>>> Here is your patch amended for that.
>>
>> I am fine with this mechanism as well.
> 
> Committed.
> 
> I also wrote a bit of documentation about slot handling for
> subscriptions, covering some of what was discussed in this thread.
> 

Great, thanks.

Here is rebased version of the other patch (the interface rework). I
also fixed the tab completion there.

-- 
  Petr Jelinek                  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, 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 Mon, May 08, 2017 at 05:01:00PM -0400, Peter Eisentraut wrote:
> On 5/7/17 04:21, Noah Misch wrote:
> > The above-described topic is currently a PostgreSQL 10 open item.  Peter,
> > since you committed the patch believed to have created it, you own this open
> > item.  If some other commit is more relevant or if this does not belong as a
> > v10 open item, please let us know.  Otherwise, please observe the policy on
> > open item ownership[1] and send a status update within three calendar days of
> > this message.  Include a date for your subsequent status update.  Testers may
> > discover new open items at any time, and I want to plan to get them all fixed
> > well in advance of shipping v10.  Consequently, I will appreciate your efforts
> > toward speedy resolution.  Thanks.
> 
> I think we have a workable patch, but it needs some rebasing.  I hope to
> have this sorted by Wednesday.

This PostgreSQL 10 open item is past due for your status update.  Kindly send
a status update within 24 hours, and include a date for your subsequent status
update.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

On 11/05/17 09:27, Noah Misch wrote:
> On Mon, May 08, 2017 at 05:01:00PM -0400, Peter Eisentraut wrote:
>> On 5/7/17 04:21, Noah Misch wrote:
>>> The above-described topic is currently a PostgreSQL 10 open item.  Peter,
>>> since you committed the patch believed to have created it, you own this open
>>> item.  If some other commit is more relevant or if this does not belong as a
>>> v10 open item, please let us know.  Otherwise, please observe the policy on
>>> open item ownership[1] and send a status update within three calendar days of
>>> this message.  Include a date for your subsequent status update.  Testers may
>>> discover new open items at any time, and I want to plan to get them all fixed
>>> well in advance of shipping v10.  Consequently, I will appreciate your efforts
>>> toward speedy resolution.  Thanks.
>>
>> I think we have a workable patch, but it needs some rebasing.  I hope to
>> have this sorted by Wednesday.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 

The patch Peter mentioned was committed.

There is however another open item associated with this thread for which
there is another patch as well.

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services

On 5/9/17 11:43, Petr Jelinek wrote:
> Here is rebased version of the other patch (the interface rework). I
> also fixed the tab completion there.

Committed.

One small thing I changed, to make it look more like CREATE/ALTER TABLE,
is that the CREATE commands use WITH (...) but the ALTER commands use
SET (...).

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

On 12/05/17 15:02, Peter Eisentraut wrote:
> On 5/9/17 11:43, Petr Jelinek wrote:
>> Here is rebased version of the other patch (the interface rework). I
>> also fixed the tab completion there.
> 
> Committed.
> 
> One small thing I changed, to make it look more like CREATE/ALTER TABLE,
> is that the CREATE commands use WITH (...) but the ALTER commands use
> SET (...).
> 

Makes sense, thanks!

--  Petr Jelinek                  http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training &
Services