Thread: [HACKERS] SUBSCRIPTIONS and pg_upgrade

[HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter_e@gmx.net) wrote:
> Logical replication
>
> - Add PUBLICATION catalogs and DDL
> - Add SUBSCRIPTION catalog and DDL
> - Define logical replication protocol and output plugin
> - Add logical replication workers

I'm not entirely sure about the reasoning behind requiring a flag to
include subscriptions in pg_dump output, as the documentation doesn't
actually provide one, but if we are going to require that, shouldn't
pg_upgrade use it, to make sure that the subscriptions are pulled
forward to the upgraded cluster?

Also, we should probably discuss that default in pg_dump to not include
something in the database by default as that's not something we've ever
done before.  We made a very deliberate point to make sure that RLS
didn't work by default with pg_dump to avoid the risk that we might not
dump include everything in the database in the pg_dump output.  I agree
that it's not exactly the same, but even so, I was surprised to find
out that subscriptions aren't included by default and I doubt I'd be
alone.

If this was all already discussed, I'm happy to go review the relevant
thread(s).  I'll admit that I didn't follow all of that discussion very
closely, I'm just going through parts of pg_dump which are not being
tested in our regression tests currently and discovered that dumping out
subscriptions is not tested at all.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Stephen Frost (sfrost@snowman.net) wrote:
> * Peter Eisentraut (peter_e@gmx.net) wrote:
> > Logical replication
> >
> > - Add PUBLICATION catalogs and DDL
> > - Add SUBSCRIPTION catalog and DDL
> > - Define logical replication protocol and output plugin
> > - Add logical replication workers
>
> I'm not entirely sure about the reasoning behind requiring a flag to
> include subscriptions in pg_dump output, as the documentation doesn't
> actually provide one, but if we are going to require that, shouldn't
> pg_upgrade use it, to make sure that the subscriptions are pulled
> forward to the upgraded cluster?
>
> Also, we should probably discuss that default in pg_dump to not include
> something in the database by default as that's not something we've ever
> done before.  We made a very deliberate point to make sure that RLS
> didn't work by default with pg_dump to avoid the risk that we might not
> dump include everything in the database in the pg_dump output.  I agree
> that it's not exactly the same, but even so, I was surprised to find
> out that subscriptions aren't included by default and I doubt I'd be
> alone.
>
> If this was all already discussed, I'm happy to go review the relevant
> thread(s).  I'll admit that I didn't follow all of that discussion very
> closely, I'm just going through parts of pg_dump which are not being
> tested in our regression tests currently and discovered that dumping out
> subscriptions is not tested at all.

We should probably also throw an error if --include-subscriptions and
--data-only are used together, instead of just not dumping out the
subscriptions in that case, which is what happens now.

Surely, if the user asked for subscriptions to be included, we should
either throw an error or actually include them.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 2/16/17 21:04, Stephen Frost wrote:
> I'm not entirely sure about the reasoning behind requiring a flag to
> include subscriptions in pg_dump output,

One reason is that pg_subscription is only readable by a superuser, so
we can't even dump them unless we're superuser.

Also, restoring a subscription will immediately attempt to start
replication, which might be kind of surprising.

We had pondered this issue extensively during early review.  I don't
think we're all happy with the current behavior, but it's probably the
safest and easiest one for now.

Better ideas are welcome.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 2/16/17 21:04, Stephen Frost wrote:
> > I'm not entirely sure about the reasoning behind requiring a flag to
> > include subscriptions in pg_dump output,
>
> One reason is that pg_subscription is only readable by a superuser, so
> we can't even dump them unless we're superuser.

Sure, but that's true of roles and other things..  On a system with RLS,
likely only the database superuser can actually read everything.

> Also, restoring a subscription will immediately attempt to start
> replication, which might be kind of surprising.

Isn't that what the 'disabled' option is for?  Also, when it comes to
pg_upgrade, isn't that what you would probably want?

Perhaps it shouldn't start immediately, but rather create them as
disabled at first and then start them at the end of the restore.

> We had pondered this issue extensively during early review.  I don't
> think we're all happy with the current behavior, but it's probably the
> safest and easiest one for now.
>
> Better ideas are welcome.

I don't think we can simply punt entirely on this, particularly for the
pg_upgrade case.  At the least, I think we should be exporting the
subscriptions in a disabled fashion, so that at least the user *has*
them in the backup, even if they aren't enabled.  With pg_upgrade, I
would think we'd want to have some option to tell pg_upgrade to include
subscriptions, or to enable them afterwards if they're included but off
by default.

I've added it to the open items for PG10.  Perhaps we can get some other
input/suggestions on it.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 2/17/17 09:33, Stephen Frost wrote:
>> One reason is that pg_subscription is only readable by a superuser, so
>> we can't even dump them unless we're superuser.
> Sure, but that's true of roles and other things..  On a system with RLS,
> likely only the database superuser can actually read everything.

There are some concurrent discussions about getting a list of
subscriptions without having superuser.  Once that is resolved, we'll
likely have more options here, such as dumping only subscriptions you
are owner of or something like that.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 2/17/17 09:33, Stephen Frost wrote:
> >> One reason is that pg_subscription is only readable by a superuser, so
> >> we can't even dump them unless we're superuser.
> > Sure, but that's true of roles and other things..  On a system with RLS,
> > likely only the database superuser can actually read everything.
>
> There are some concurrent discussions about getting a list of
> subscriptions without having superuser.  Once that is resolved, we'll
> likely have more options here, such as dumping only subscriptions you
> are owner of or something like that.

I certainly agree that we should provide a way to view subscriptions,
and perhaps do other things with them, without being a superuser.

I was going to suggest that it might make sense to have a role attribute
or a default role created for managing publications/subscriptions.  It
seems like this should be independent of the existing replicaiton role
attribute, which is about physical replication.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Robert Haas
Date:
On Fri, Feb 17, 2017 at 7:34 AM, Stephen Frost <sfrost@snowman.net> wrote:
> I'm not entirely sure about the reasoning behind requiring a flag to
> include subscriptions in pg_dump output, as the documentation doesn't
> actually provide one, but if we are going to require that, shouldn't
> pg_upgrade use it, to make sure that the subscriptions are pulled
> forward to the upgraded cluster?

Subscriptions are different from other objects in that whether or not
you want them in your dump output depends on how you plan to use the
dump.  If your goal is to create a server to replace the original
server, you want the subscriptions.  If you goal is to take a static
copy of the data, you don't.  Subscriptions aren't really data in the
sense that table data is data, or even in the sense that functions are
data.  Granted, you can execute a function, but a subscription is
self-executing.

That having been said, I share your discomfort with not dumping these
by default.  I think it would be good to dump and restore them by
default, but maybe restore them in disabled mode as you suggest
downthread, and document that if you want them enabled you have to
turn them on after doing the restore.  Otherwise, you could have
logical replication start up before the dump restore even completes,
which seems like it could cause all sorts of problems.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Noah Misch
Date:
On Fri, Feb 17, 2017 at 09:33:32AM -0500, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> > On 2/16/17 21:04, Stephen Frost wrote:
> > > I'm not entirely sure about the reasoning behind requiring a flag to
> > > include subscriptions in pg_dump output,
> > 
> > One reason is that pg_subscription is only readable by a superuser, so
> > we can't even dump them unless we're superuser.
> 
> Sure, but that's true of roles and other things..  On a system with RLS,
> likely only the database superuser can actually read everything.
> 
> > Also, restoring a subscription will immediately attempt to start
> > replication, which might be kind of surprising.
> 
> Isn't that what the 'disabled' option is for?  Also, when it comes to
> pg_upgrade, isn't that what you would probably want?
> 
> Perhaps it shouldn't start immediately, but rather create them as
> disabled at first and then start them at the end of the restore.
> 
> > We had pondered this issue extensively during early review.  I don't
> > think we're all happy with the current behavior, but it's probably the
> > safest and easiest one for now.
> > 
> > Better ideas are welcome.
> 
> I don't think we can simply punt entirely on this, particularly for the
> pg_upgrade case.  At the least, I think we should be exporting the
> subscriptions in a disabled fashion, so that at least the user *has*
> them in the backup, even if they aren't enabled.  With pg_upgrade, I
> would think we'd want to have some option to tell pg_upgrade to include
> subscriptions, or to enable them afterwards if they're included but off
> by default.
> 
> I've added it to the open items for PG10.  Perhaps we can get some other
> input/suggestions on it.

[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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
OK, we need to come to a conclusion here.  To summarize:

Problem 1: pg_subscription.subconninfo can only be read by superuser.
So non-superusers cannot dump subscriptions.

Precedent is pg_user_mapping.  In that case, we just omit the
user-mapping options if we're not a superuser.  Pretty dubious, but in
any case that won't work here, because you cannot create a subscription
without a CONNECTION clause.

Proposal: Dump subscriptions if running as superuser.  If not, check if
there are subscriptions and warn about that.  Remove current pg_dump
--include-subscriptions option.


Problem 2: Restoring a subscription immediately starts replication.
Maybe you want that or maybe you don't.  We could dump all subscriptions
in DISABLED state.  But then after restoring you have to go and manually
enable all subscriptions.  We don't have a command to turn all
subscriptions back on at once.  Maybe that is not a terrible issue,
since one wouldn't normally have many subscriptions.

Proposal: Dump all subscriptions in DISABLED state.  Remove current
pg_dump --no-subscription-connect option.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> Problem 1: pg_subscription.subconninfo can only be read by superuser.
> So non-superusers cannot dump subscriptions.

I'm not particularly happy with this.

> Precedent is pg_user_mapping.  In that case, we just omit the
> user-mapping options if we're not a superuser.  Pretty dubious, but in
> any case that won't work here, because you cannot create a subscription
> without a CONNECTION clause.

Given that you can create a disabled subscription, why is a connection
clause required..?  I agree that simply excluding pg_user_mapping isn't
great but I don't really think we want to use something which we agree
isn't ideal as the best approach for this.

> Proposal: Dump subscriptions if running as superuser.  If not, check if
> there are subscriptions and warn about that.  Remove current pg_dump
> --include-subscriptions option.

That option was added, iirc, in part because pg_dump was including
subscriptions in things like explicit single-table dumps.  I certainly
don't think we should start including all subscriptions in all dumps.

The question becomes if it's useful to include subscriptions when only
dumping a subset of objects or if they should *only* be included when
doing a full dump.  My gut feeling is that if we care enough about blobs
to have an explicit option to include them, even when we aren't dumping
out everything, then having something similar for subscriptions makes
sense.

> Problem 2: Restoring a subscription immediately starts replication.
> Maybe you want that or maybe you don't.  We could dump all subscriptions
> in DISABLED state.  But then after restoring you have to go and manually
> enable all subscriptions.  We don't have a command to turn all
> subscriptions back on at once.  Maybe that is not a terrible issue,
> since one wouldn't normally have many subscriptions.

Having a way to turn them all on would be nice.

> Proposal: Dump all subscriptions in DISABLED state.  Remove current
> pg_dump --no-subscription-connect option.

I like this idea in general, I'm just not sure if it's the right answer
when we're talking about pg_upgrade.  At a minimum, if we go with this
approach, we should produce a warning when subscriptions exist during
the pg_upgrade that the user will need to go re-enable them.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/10/17 20:55, Stephen Frost wrote:
> Peter,
> 
> * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
>> Problem 1: pg_subscription.subconninfo can only be read by superuser.
>> So non-superusers cannot dump subscriptions.
> 
> I'm not particularly happy with this.

Why?  How?  Alternatives?

>> Precedent is pg_user_mapping.  In that case, we just omit the
>> user-mapping options if we're not a superuser.  Pretty dubious, but in
>> any case that won't work here, because you cannot create a subscription
>> without a CONNECTION clause.
> 
> Given that you can create a disabled subscription, why is a connection
> clause required..?  I agree that simply excluding pg_user_mapping isn't
> great but I don't really think we want to use something which we agree
> isn't ideal as the best approach for this.

Well, I suppose you could somehow make it work so that a connection
clause is not required.  But then why dump the subscription at all?  You
start stripping off information and it becomes less useful.

>> Proposal: Dump subscriptions if running as superuser.  If not, check if
>> there are subscriptions and warn about that.  Remove current pg_dump
>> --include-subscriptions option.
> 
> That option was added, iirc, in part because pg_dump was including
> subscriptions in things like explicit single-table dumps.

No, you are thinking of publications.

The option was added because at some point during the review process of
the early patches, pg_dump for a non-superuser would just fail outright
as it was trying to read pg_subscription.

> The question becomes if it's useful to include subscriptions when only
> dumping a subset of objects or if they should *only* be included when
> doing a full dump.

I think we'd handle that similar to publications.

>> Proposal: Dump all subscriptions in DISABLED state.  Remove current
>> pg_dump --no-subscription-connect option.
> 
> I like this idea in general, I'm just not sure if it's the right answer
> when we're talking about pg_upgrade.  At a minimum, if we go with this
> approach, we should produce a warning when subscriptions exist during
> the pg_upgrade that the user will need to go re-enable them.

Sure, that's doable.  It's the way of pg_upgrade in general to give you
a bunch of notes and scripts afterwards, so it wouldn't be too strange
to add that in.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 4/10/17 20:55, Stephen Frost wrote:
> > * Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> >> Problem 1: pg_subscription.subconninfo can only be read by superuser.
> >> So non-superusers cannot dump subscriptions.
> >
> > I'm not particularly happy with this.
>
> Why?  How?  Alternatives?

Generally speaking, we should be trying to move away from superuser-only
anything, not introducing more of it.  If the connection string can have
sensitive data in it, I'd be happier if we could pull that sensitive
information out into its own column to allow the rest to be included,
but if we have to exclude the connection string then we have to, but the
rest should be available to individuals with appropriate rights.  That
doesn't necessairly mean "public" should be allowed to see it, but we
should have a role who can who isn't superuser.

> >> Precedent is pg_user_mapping.  In that case, we just omit the
> >> user-mapping options if we're not a superuser.  Pretty dubious, but in
> >> any case that won't work here, because you cannot create a subscription
> >> without a CONNECTION clause.
> >
> > Given that you can create a disabled subscription, why is a connection
> > clause required..?  I agree that simply excluding pg_user_mapping isn't
> > great but I don't really think we want to use something which we agree
> > isn't ideal as the best approach for this.
>
> Well, I suppose you could somehow make it work so that a connection
> clause is not required.  But then why dump the subscription at all?  You
> start stripping off information and it becomes less useful.

Isn't there other information associated with the subscription beyond
just the connection string?  Even if there isn't today, I certainly
expect there will be in the future (at a minimum, mapping between the
foreign table and a local table with a different name really should be
supported...).

Consider the recently added option to not include passwords for roles
being dumped through pg_dumpall.  That's useful, for multiple reasons,
even if the roles don't have any objects which depend on them.
Including this information is required to rebuild the system as closely
as possible to the original state.

> >> Proposal: Dump subscriptions if running as superuser.  If not, check if
> >> there are subscriptions and warn about that.  Remove current pg_dump
> >> --include-subscriptions option.
> >
> > That option was added, iirc, in part because pg_dump was including
> > subscriptions in things like explicit single-table dumps.
>
> No, you are thinking of publications.

Even so, the other comments apply equally here, from what I can tell.

> The option was added because at some point during the review process of
> the early patches, pg_dump for a non-superuser would just fail outright
> as it was trying to read pg_subscription.

That's certainly an issue in general.

> > The question becomes if it's useful to include subscriptions when only
> > dumping a subset of objects or if they should *only* be included when
> > doing a full dump.
>
> I think we'd handle that similar to publications.

Which is to say that they'd only be included in a 'full' dump?  I'd like
a way to do better than that in general, but having them only included
in full dumps would also be an option, at least for PG10.

> >> Proposal: Dump all subscriptions in DISABLED state.  Remove current
> >> pg_dump --no-subscription-connect option.
> >
> > I like this idea in general, I'm just not sure if it's the right answer
> > when we're talking about pg_upgrade.  At a minimum, if we go with this
> > approach, we should produce a warning when subscriptions exist during
> > the pg_upgrade that the user will need to go re-enable them.
>
> Sure, that's doable.  It's the way of pg_upgrade in general to give you
> a bunch of notes and scripts afterwards, so it wouldn't be too strange
> to add that in.

Right, that's why I was suggesting it.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Robert Haas
Date:
On Mon, Apr 10, 2017 at 10:08 PM, Stephen Frost <sfrost@snowman.net> wrote:
> Generally speaking, we should be trying to move away from superuser-only
> anything, not introducing more of it.

I totally agree, which is why I was rather surprised when you
vigorously objected to my attempts to replace the remainder of the
main tree's superuser checks that completely block execution of
certain SQL functions with privilege grants.  The parameters within
which you find explicit superuser checks tolerable are extremely murky
to me.

> If the connection string can have
> sensitive data in it, ...

I would argue that a great deal of what's in a connection string could
potentially be sensitive.  Do you want to disclose to unprivileged
users potentially-useful host names, IP addresses, port numbers, user
names, passwords, and/or required SSL settings?  I don't think we
should assume any of that stuff to be generally OK to disclose to
non-superusers.  It could be OK to disclose to everyone in some
installations, or to some people even in highly secure installations,
but the idea that there is nobody who cares about obscuring the
majority of what goes into a connection string doesn't sound right to
me.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Robert,

* Robert Haas (robertmhaas@gmail.com) wrote:
> On Mon, Apr 10, 2017 at 10:08 PM, Stephen Frost <sfrost@snowman.net> wrote:
> > Generally speaking, we should be trying to move away from superuser-only
> > anything, not introducing more of it.
>
> I totally agree, which is why I was rather surprised when you
> vigorously objected to my attempts to replace the remainder of the
> main tree's superuser checks that completely block execution of
> certain SQL functions with privilege grants.  The parameters within
> which you find explicit superuser checks tolerable are extremely murky
> to me.

Filesystem-level access seems reasonable to restrict to superusers.

> > If the connection string can have
> > sensitive data in it, ...
>
> I would argue that a great deal of what's in a connection string could
> potentially be sensitive.  Do you want to disclose to unprivileged
> users potentially-useful host names, IP addresses, port numbers, user
> names, passwords, and/or required SSL settings?  I don't think we
> should assume any of that stuff to be generally OK to disclose to
> non-superusers.  It could be OK to disclose to everyone in some
> installations, or to some people even in highly secure installations,
> but the idea that there is nobody who cares about obscuring the
> majority of what goes into a connection string doesn't sound right to
> me.

I specifically made a point to not question what was or wasn't
considered sensitive as it can certainly vary.  The point I was making
is that if there's sensitive information there then we can exclude just
that information but allow a pg_dump-using user to see that a
subscription exists without it.

I agree that it might be interesting to discuss which information should
be limited to superusers only, which information should be available to
privileged non-superusers (pg_read_all_settings, for example, allows
visibility to information that we previously limited to superusers) and
what information should be available to the 'public' user, but this
isn't the place to discuss any of that- this is about how to address the
issues which currently exist around pg_dump'ing of subscriptions (and,
perhaps, publications and they're related).  I don't believe we want to
de-rail this into a largely independent discussion, given that it's an
open item that needs to be addressed shortly if we're going to get beta
out on time.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Robert Haas
Date:
On Tue, Apr 11, 2017 at 9:48 AM, Robert Haas <robertmhaas@gmail.com> wrote:
> On Mon, Apr 10, 2017 at 1:58 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> OK, we need to come to a conclusion here.  To summarize:
>>
>> Problem 1: pg_subscription.subconninfo can only be read by superuser.
>> So non-superusers cannot dump subscriptions.
>>
>> Precedent is pg_user_mapping.  In that case, we just omit the
>> user-mapping options if we're not a superuser.  Pretty dubious, but in
>> any case that won't work here, because you cannot create a subscription
>> without a CONNECTION clause.
>>
>> Proposal: Dump subscriptions if running as superuser.  If not, check if
>> there are subscriptions and warn about that.  Remove current pg_dump
>> --include-subscriptions option.
>
> +1. I don't totally love it, but I don't have a better idea.
>
>> Problem 2: Restoring a subscription immediately starts replication.
>> Maybe you want that or maybe you don't.  We could dump all subscriptions
>> in DISABLED state.  But then after restoring you have to go and manually
>> enable all subscriptions.  We don't have a command to turn all
>> subscriptions back on at once.  Maybe that is not a terrible issue,
>> since one wouldn't normally have many subscriptions.
>>
>> Proposal: Dump all subscriptions in DISABLED state.  Remove current
>> pg_dump --no-subscription-connect option.
>
> +1. I like this a lot.

Oops, forgot to copy the list.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Robert Haas
Date:
On Tue, Apr 11, 2017 at 10:13 AM, Stephen Frost <sfrost@snowman.net> wrote:
>> I totally agree, which is why I was rather surprised when you
>> vigorously objected to my attempts to replace the remainder of the
>> main tree's superuser checks that completely block execution of
>> certain SQL functions with privilege grants.  The parameters within
>> which you find explicit superuser checks tolerable are extremely murky
>> to me.
>
> Filesystem-level access seems reasonable to restrict to superusers.

I think in an ideal world we wouldn't have any hard-coded superuser()
checks at all, because all privileges would be able to be delegated in
a fine-grained manner.  But even if I were to agree with you on this
point, you've argued for a fairly inconsistent application of that
principle.  However, I do agree with your remarks later in the email
that it's best not to derail this thread to discuss that issue in
depth, so I'll shut up about this for now.

> I specifically made a point to not question what was or wasn't
> considered sensitive as it can certainly vary.  The point I was making
> is that if there's sensitive information there then we can exclude just
> that information but allow a pg_dump-using user to see that a
> subscription exists without it.

OK, apparently I misread your remarks.  I thought you were arguing
that the data wasn't sensitive, which seemed like an odd take.

I don't think it's the purpose of pg_dump to reveal what objects
exist, but rather to create a dump file that can be used to recreate
the state of the database.  To the extent that the user lacks
permissions necessary to dump the objects, they can't be dumped.
Maybe modifying subscriptions so that they can exist without a
connection and "half-dumping" them is better than not dumping them at
all, but my initial impression is to think that it's worse.  A user
may be more likely to notice something that's missing altogether than
something that exists but in a non-working state.  You mentioned
--no-role-passwords, but that's a nasty kludge in my book and I'm not
in favor of making something like that the default behavior.  Peter's
approach looks like less work, and helps us get beta out the door.

I can't claim to be an expert on all of this, though.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/10/17 13:58, Peter Eisentraut wrote:
> Proposal: Dump subscriptions if running as superuser.  If not, check if
> there are subscriptions and warn about that.  Remove current pg_dump
> --include-subscriptions option.

Patch for this part.

-- 
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

Attachment

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/10/17 20:55, Stephen Frost wrote:
>> Proposal: Dump all subscriptions in DISABLED state.  Remove current
>> pg_dump --no-subscription-connect option.
> I like this idea in general, I'm just not sure if it's the right answer
> when we're talking about pg_upgrade.  At a minimum, if we go with this
> approach, we should produce a warning when subscriptions exist during
> the pg_upgrade that the user will need to go re-enable them.

It's not clear what to do with a subscription after a dump/restore or a
pg_upgrade anyway.  You can't just continue streaming where you left
off.  Most likely, you will want to truncate the target tables and
resync them.  In some cases, you might just accept the data gap and
continue streaming at the current state.  But in any case you'll need to
decide based on what you're actually doing.  Just telling the user, turn
it back on and you're ready to go isn't necessarily the right answer.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/9/17 22:16, Noah Misch wrote:
> [Action required within three days.  This is a generic notification.]

Patches have been posted.  Discussion is still going on a bit.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Noah Misch
Date:
On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
> On 4/9/17 22:16, Noah Misch wrote:
> > [Action required within three days.  This is a generic notification.]
> 
> Patches have been posted.  Discussion is still going on a bit.

By what day should the community look for your next update?



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/11/17 23:41, Noah Misch wrote:
> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>> On 4/9/17 22:16, Noah Misch wrote:
>>> [Action required within three days.  This is a generic notification.]
>>
>> Patches have been posted.  Discussion is still going on a bit.
> 
> By what day should the community look for your next update?

tomorrow

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/11/17 22:16, Peter Eisentraut wrote:
> On 4/10/17 13:58, Peter Eisentraut wrote:
>> Proposal: Dump subscriptions if running as superuser.  If not, check if
>> there are subscriptions and warn about that.  Remove current pg_dump
>> --include-subscriptions option.
> 
> Patch for this part.

And patch for the second part.

I'll commit those in a day or two if there are no new insights.

-- 
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

Attachment

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/12/17 18:31, Peter Eisentraut wrote:
> On 4/11/17 23:41, Noah Misch wrote:
>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>>> On 4/9/17 22:16, Noah Misch wrote:
>>>> [Action required within three days.  This is a generic notification.]
>>>
>>> Patches have been posted.  Discussion is still going on a bit.
>>
>> By what day should the community look for your next update?
> 
> tomorrow

Everything has been committed, and this thread can be closed.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Robert Haas
Date:
On Thu, Apr 13, 2017 at 12:05 PM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 4/12/17 18:31, Peter Eisentraut wrote:
>> On 4/11/17 23:41, Noah Misch wrote:
>>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>>>> On 4/9/17 22:16, Noah Misch wrote:
>>>>> [Action required within three days.  This is a generic notification.]
>>>>
>>>> Patches have been posted.  Discussion is still going on a bit.
>>>
>>> By what day should the community look for your next update?
>>
>> tomorrow
>
> Everything has been committed, and this thread can be closed.

I wonder if we should have an --no-subscriptions option, now that they
are dumped by default, just like we have --no-blobs, --no-owner,
--no-password, --no-privileges, --no-acl, --no-tablespaces, and
--no-security-labels.  It seems like there is probably a fairly large
use case for excluding subscriptions even if you have sufficient
permissions to dump them.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Petr Jelinek
Date:
On 13/04/17 18:11, Robert Haas wrote:
> On Thu, Apr 13, 2017 at 12:05 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 4/12/17 18:31, Peter Eisentraut wrote:
>>> On 4/11/17 23:41, Noah Misch wrote:
>>>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
>>>>> On 4/9/17 22:16, Noah Misch wrote:
>>>>>> [Action required within three days.  This is a generic notification.]
>>>>>
>>>>> Patches have been posted.  Discussion is still going on a bit.
>>>>
>>>> By what day should the community look for your next update?
>>>
>>> tomorrow
>>
>> Everything has been committed, and this thread can be closed.
> 
> I wonder if we should have an --no-subscriptions option, now that they
> are dumped by default, just like we have --no-blobs, --no-owner,
> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> --no-security-labels.  It seems like there is probably a fairly large
> use case for excluding subscriptions even if you have sufficient
> permissions to dump them.
> 

+1, I'll look into writing patch for that

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 4/13/17 12:11, Robert Haas wrote:
> I wonder if we should have an --no-subscriptions option, now that they
> are dumped by default, just like we have --no-blobs, --no-owner,
> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> --no-security-labels.  It seems like there is probably a fairly large
> use case for excluding subscriptions even if you have sufficient
> permissions to dump them.

What purpose would that serve in practice?  The subscriptions are now
dumped disabled, so if they hang around, there is not much impact.

Perhaps an option that also omits publications would make sense?

Or a general filtering facility based on the object tag?

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Stephen Frost
Date:
Peter,

* Peter Eisentraut (peter.eisentraut@2ndquadrant.com) wrote:
> On 4/13/17 12:11, Robert Haas wrote:
> > I wonder if we should have an --no-subscriptions option, now that they
> > are dumped by default, just like we have --no-blobs, --no-owner,
> > --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> > --no-security-labels.  It seems like there is probably a fairly large
> > use case for excluding subscriptions even if you have sufficient
> > permissions to dump them.
>
> What purpose would that serve in practice?  The subscriptions are now
> dumped disabled, so if they hang around, there is not much impact.

Will they be allowed to be created as a non-superuser when loading into
another cluster?  Even if you might have superuser rights on one
cluster, you may not on another.

> Perhaps an option that also omits publications would make sense?

Yes.

> Or a general filtering facility based on the object tag?

Perhaps..  I'd hate for it to end up being overly complicated though.
The general '--no-security-lables', '--no-tablespaces' and other options
are pretty straight-forward and I think that's a good thing.

Thanks!

Stephen

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Noah Misch
Date:
On Thu, Apr 13, 2017 at 12:11:30PM -0400, Robert Haas wrote:
> On Thu, Apr 13, 2017 at 12:05 PM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
> > On 4/12/17 18:31, Peter Eisentraut wrote:
> >> On 4/11/17 23:41, Noah Misch wrote:
> >>> On Tue, Apr 11, 2017 at 11:21:24PM -0400, Peter Eisentraut wrote:
> >>>> On 4/9/17 22:16, Noah Misch wrote:
> >>>>> [Action required within three days.  This is a generic notification.]
> >>>>
> >>>> Patches have been posted.  Discussion is still going on a bit.
> >>>
> >>> By what day should the community look for your next update?
> >>
> >> tomorrow
> >
> > Everything has been committed, and this thread can be closed.
> 
> I wonder if we should have an --no-subscriptions option, now that they
> are dumped by default, just like we have --no-blobs, --no-owner,
> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> --no-security-labels.  It seems like there is probably a fairly large
> use case for excluding subscriptions even if you have sufficient
> permissions to dump them.

[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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 5/2/17 21:44, Noah Misch wrote:
>> I wonder if we should have an --no-subscriptions option, now that they
>> are dumped by default, just like we have --no-blobs, --no-owner,
>> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
>> --no-security-labels.  It seems like there is probably a fairly large
>> use case for excluding subscriptions even if you have sufficient
>> permissions to dump them.
> 
> [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.

I consider this item low priority and don't plan to work on it before
all the other open items under logical replication are addressed.

(Here, working on it would include thinking further about whether it is
necessary at all or what alternatives might look like.)

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Noah Misch
Date:
On Fri, May 05, 2017 at 11:01:57AM -0400, Peter Eisentraut wrote:
> On 5/2/17 21:44, Noah Misch wrote:
> >> I wonder if we should have an --no-subscriptions option, now that they
> >> are dumped by default, just like we have --no-blobs, --no-owner,
> >> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> >> --no-security-labels.  It seems like there is probably a fairly large
> >> use case for excluding subscriptions even if you have sufficient
> >> permissions to dump them.
> > 
> > [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.
> 
> I consider this item low priority and don't plan to work on it before
> all the other open items under logical replication are addressed.
> 
> (Here, working on it would include thinking further about whether it is
> necessary at all or what alternatives might look like.)

That's informative, but it's not a valid status update.  This PostgreSQL 10
open item is past due for your status update.  Kindly send a valid status
update within 24 hours.  Refer to the policy on open item ownership:
https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Noah Misch
Date:
On Sat, May 06, 2017 at 11:50:16AM -0700, Noah Misch wrote:
> On Fri, May 05, 2017 at 11:01:57AM -0400, Peter Eisentraut wrote:
> > On 5/2/17 21:44, Noah Misch wrote:
> > >> I wonder if we should have an --no-subscriptions option, now that they
> > >> are dumped by default, just like we have --no-blobs, --no-owner,
> > >> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
> > >> --no-security-labels.  It seems like there is probably a fairly large
> > >> use case for excluding subscriptions even if you have sufficient
> > >> permissions to dump them.
> > > 
> > > [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.
> > 
> > I consider this item low priority and don't plan to work on it before
> > all the other open items under logical replication are addressed.
> > 
> > (Here, working on it would include thinking further about whether it is
> > necessary at all or what alternatives might look like.)
> 
> That's informative, but it's not a valid status update.  This PostgreSQL 10
> open item is past due for your status update.  Kindly send a valid status
> update within 24 hours.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

IMMEDIATE ATTENTION REQUIRED.  This PostgreSQL 10 open item is long past due
for your status update.  Please reacquaint yourself with the policy on open
item ownership[1] and then reply immediately.  If I do not hear from you by
2017-05-09 07:00 UTC, I will transfer this item to release management team
ownership without further notice.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 5/6/17 14:50, Noah Misch wrote:
>> I consider this item low priority and don't plan to work on it before
>> all the other open items under logical replication are addressed.
>>
>> (Here, working on it would include thinking further about whether it is
>> necessary at all or what alternatives might look like.)
> 
> That's informative, but it's not a valid status update.  This PostgreSQL 10
> open item is past due for your status update.  Kindly send a valid status
> update within 24 hours.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

Fair enough.  I have set myself a reminder to report back on May 30.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Michael Paquier
Date:
On Sat, May 6, 2017 at 12:01 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/2/17 21:44, Noah Misch wrote:
>>> I wonder if we should have an --no-subscriptions option, now that they
>>> are dumped by default, just like we have --no-blobs, --no-owner,
>>> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
>>> --no-security-labels.  It seems like there is probably a fairly large
>>> use case for excluding subscriptions even if you have sufficient
>>> permissions to dump them.

Reading the thread for the first time, I am +1 on that. It feels more
natural to have subscriptions by default when taking a dump, and it is
useful as well to be able to override the default so as they are not
included.

On Tue, May 9, 2017 at 1:26 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/6/17 14:50, Noah Misch wrote:
>>> I consider this item low priority and don't plan to work on it before
>>> all the other open items under logical replication are addressed.
>>>
>>> (Here, working on it would include thinking further about whether it is
>>> necessary at all or what alternatives might look like.)
>>
>> That's informative, but it's not a valid status update.  This PostgreSQL 10
>> open item is past due for your status update.  Kindly send a valid status
>> update within 24 hours.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>
> Fair enough.  I have set myself a reminder to report back on May 30.

I think that it would be nice to fix that even before beta, so
attached is a patch to add --no-subscriptions to pg_dump, pg_dumpall
and pg_restore.
-- 
Michael

-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

Attachment

Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Petr Jelinek
Date:
On 09/05/17 07:24, Michael Paquier wrote:
> On Sat, May 6, 2017 at 12:01 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 5/2/17 21:44, Noah Misch wrote:
>>>> I wonder if we should have an --no-subscriptions option, now that they
>>>> are dumped by default, just like we have --no-blobs, --no-owner,
>>>> --no-password, --no-privileges, --no-acl, --no-tablespaces, and
>>>> --no-security-labels.  It seems like there is probably a fairly large
>>>> use case for excluding subscriptions even if you have sufficient
>>>> permissions to dump them.
> 
> Reading the thread for the first time, I am +1 on that. It feels more
> natural to have subscriptions by default when taking a dump, and it is
> useful as well to be able to override the default so as they are not
> included.

Yes I agree. There was also talk of generalizing the --no-* stuff so we
could filter arbitrary objects which is something I would prefer as
solution, but at this point that's potential PG11 feature not PG10 one.

> 
> On Tue, May 9, 2017 at 1:26 AM, Peter Eisentraut
> <peter.eisentraut@2ndquadrant.com> wrote:
>> On 5/6/17 14:50, Noah Misch wrote:
>>>> I consider this item low priority and don't plan to work on it before
>>>> all the other open items under logical replication are addressed.
>>>>
>>>> (Here, working on it would include thinking further about whether it is
>>>> necessary at all or what alternatives might look like.)
>>>
>>> That's informative, but it's not a valid status update.  This PostgreSQL 10
>>> open item is past due for your status update.  Kindly send a valid status
>>> update within 24 hours.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>>
>> Fair enough.  I have set myself a reminder to report back on May 30.
> 
> I think that it would be nice to fix that even before beta, so
> attached is a patch to add --no-subscriptions to pg_dump, pg_dumpall
> and pg_restore.
> 

Looks okay to me, it's simple enough patch.

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Peter Eisentraut
Date:
On 5/9/17 04:54, Petr Jelinek wrote:
>> I think that it would be nice to fix that even before beta, so
>> attached is a patch to add --no-subscriptions to pg_dump, pg_dumpall
>> and pg_restore.
> 
> Looks okay to me, it's simple enough patch.

Committed, thanks.

(There was some inconsistent variable naming no_subscription vs
no_subscriptions, which I sorted out.)

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



Re: [HACKERS] SUBSCRIPTIONS and pg_upgrade

From
Michael Paquier
Date:
On Wed, May 10, 2017 at 12:00 AM, Peter Eisentraut
<peter.eisentraut@2ndquadrant.com> wrote:
> On 5/9/17 04:54, Petr Jelinek wrote:
>>> I think that it would be nice to fix that even before beta, so
>>> attached is a patch to add --no-subscriptions to pg_dump, pg_dumpall
>>> and pg_restore.
>>
>> Looks okay to me, it's simple enough patch.
>
> Committed, thanks.
>
> (There was some inconsistent variable naming no_subscription vs
> no_subscriptions, which I sorted out.)

Thanks.
-- 
Michael