Re: SUBSCRIPTIONS and pg_upgrade

Peter,

* Peter Eisentraut (peter(dot)eisentraut(at)2ndquadrant(dot)com) wrote:
> On 4/10/17 20:55, Stephen Frost wrote:
> > * Peter Eisentraut (peter(dot)eisentraut(at)2ndquadrant(dot)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