Special role for subscriptions

Greetings,

* Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

In postgresql 10 and 11 only superuser can create/alter subscriptions.
If there was a special role (like pg_monitor), it would be more easy to grant control on subscriptions.
I can make a patch if there are no objections against it.

I think the short answer is 'yes, we should let non-superusers do that',
but the longer answer is:

What level of access makes sense for managing subscriptions? Should
there be a way to say "user X is allowed to create a subscription for
remote system Y, but only for tables that exist in schema Q"?

My general feeling is 'yes', though, of course, I don't want to say that
we have to have all of that before we move forward with allowing
non-superusers to create subscriptions, but I do think we want to make
sure that we have a well thought-out path for how to get from where we
are now to a system which has a lot more granularity, and to do our best
to try avoiding any paths that might paint us into a corner.

Thanks!

Stephen

Hi!
As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

03.11.2018, 19:20, "Stephen Frost" <sfrost@snowman.net>:

Greetings,

* Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

That's a nice idea but seems like we would want to have a way to filter
what tables a subscription follows then..? Just failing if the
publication publishes tables that we don't have access to or are not the
owner of seems like a poor solution..

Thanks!

Stephen

Hi!
I think we can add FOR TABLES clause for create/refresh subscription, for example: CREATE SUBSCRIPTION my_sub CONNECTION ... PUBLICATION my_pub [WITH ...] [ FOR TABLES t1, t2 | ALL TABLES ]. ALL TABLES is avalibale only for superuser. FOR TABLES t1, t2 is available to owner of tables and superuser.

07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:

Greetings,

* Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

 As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

That's a nice idea but seems like we would want to have a way to filter
what tables a subscription follows then..? Just failing if the
publication publishes tables that we don't have access to or are not the
owner of seems like a poor solution..

Thanks!

Stephen

--------
Ефимкин Евгений

Hi!
In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriber side:
1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
```
CREATE SUBSCRIPTION subscription_name
CONNECTION 'conninfo'
PUBLICATION publication_name [, ...]
[ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
[ WITH ( subscription_parameter [= value] [, ... ] ) ]
```
... where `FOR ALL TABLES` is only allowed for superuser.
and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?)

2. Each subscription should have "all tables" attribute.
For example via a new column in pg_subscription "suballtables".

3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
```
ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data];
ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
```
4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The check is ommited if subscription owner is superuser.
5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser owner, we should filter tables which owner is not subscription's owner or maybe we need to raise error?

What do you think about it? Any objections?

07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:

Greetings,

* Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

 As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

That's a nice idea but seems like we would want to have a way to filter
what tables a subscription follows then..? Just failing if the
publication publishes tables that we don't have access to or are not the
owner of seems like a poor solution..

Thanks!

Stephen

--------
Ефимкин Евгений

Hello!
I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`.
I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication).
New column in pg_subscription (suballtables) will be used in `REFRESH` clause

09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

Hi!
In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriber side:
    1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
       ```
        CREATE SUBSCRIPTION subscription_name
            CONNECTION 'conninfo'
            PUBLICATION publication_name [, ...]
            [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
            [ WITH ( subscription_parameter [= value] [, ... ] ) ]
       ```
       ... where `FOR ALL TABLES` is only allowed for superuser.
       and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?)

    2. Each subscription should have "all tables" attribute.
       For example via a new column in pg_subscription "suballtables".

    3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
       ```
        ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data];
        ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
       ```
    4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The check is ommited if subscription owner is superuser.
    5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser owner, we should filter tables which owner is not subscription's owner or maybe we need to raise error?

What do you think about it? Any objections?

07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:

 Greetings,

 * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

  As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

 That's a nice idea but seems like we would want to have a way to filter
 what tables a subscription follows then..? Just failing if the
 publication publishes tables that we don't have access to or are not the
 owner of seems like a poor solution..

 Thanks!

 Stephen

--------
Ефимкин Евгений

--------
Ефимкин Евгений

Hello!
New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` for own tables.

14.11.2018, 18:10, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

Hello!
I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`.
I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication).
New column in pg_subscription (suballtables) will be used in `REFRESH` clause

09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

 Hi!
 In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriber side:
     1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
        ```
         CREATE SUBSCRIPTION subscription_name
             CONNECTION 'conninfo'
             PUBLICATION publication_name [, ...]
             [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
             [ WITH ( subscription_parameter [= value] [, ... ] ) ]
        ```
        ... where `FOR ALL TABLES` is only allowed for superuser.
        and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?)

     2. Each subscription should have "all tables" attribute.
        For example via a new column in pg_subscription "suballtables".

     3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
        ```
         ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data];
         ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
        ```
     4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The check is ommited if subscription owner is superuser.
     5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser owner, we should filter tables which owner is not subscription's owner or maybe we need to raise error?

 What do you think about it? Any objections?

 07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:

  Greetings,

  * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

   As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

  That's a nice idea but seems like we would want to have a way to filter
  what tables a subscription follows then..? Just failing if the
  publication publishes tables that we don't have access to or are not the
  owner of seems like a poor solution..

  Thanks!

  Stephen

 --------
 Ефимкин Евгений

--------
Ефимкин Евгений

--------
Ефимкин Евгений

Hello!
I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now old and new tests pass.

22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

Hello!
New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` for own tables.

14.11.2018, 18:10, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

 Hello!
 I started work on patch (draft attached). Draft has changes related only to `CREATE SUBSCRIPTION`.
 I also introduce a new status (DEFFERED) for tables in `FOR TABLE` clause (but not in publication).
 New column in pg_subscription (suballtables) will be used in `REFRESH` clause

 09.11.2018, 15:24, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

  Hi!
  In order to support create subscription from non-superuser, we need to make it possible to choose tables on the subscriber side:
      1. add `FOR TABLE` clause in `CREATE SUBSCRIPTION`:
         ```
          CREATE SUBSCRIPTION subscription_name
              CONNECTION 'conninfo'
              PUBLICATION publication_name [, ...]
              [ FOR TABLE [ ONLY ] table_name [ * ] [, ...]| FOR ALL TABLES ]
              [ WITH ( subscription_parameter [= value] [, ... ] ) ]
         ```
         ... where `FOR ALL TABLES` is only allowed for superuser.
         and table list in `FOR TABLES` clause will be stored in pg_subscription_rel table (maybe another place?)

      2. Each subscription should have "all tables" attribute.
         For example via a new column in pg_subscription "suballtables".

      3. Add `ALTER SUBSCRIPTION (ADD TABLE | DROP TABLE)`:
         ```
          ALTER SUBSCRIPTION subscription_name ADD TABLE [ ONLY ] table_name [WITH copy_data];
          ALTER SUBSCRIPTION subscription_name DROP TABLE [ ONLY ] table_name;
         ```
      4. On `ALTER SUBSCRIPTION <name> REFRESH PUBLICATION` should check if table owner equals subscription owner. The check is ommited if subscription owner is superuser.
      5. If superuser calls `ALTER SUBSCRIPTION REFRESH PUBLICATION` on subscription with table list and non-superuser owner, we should filter tables which owner is not subscription's owner or maybe we need to raise error?

  What do you think about it? Any objections?

  07.11.2018, 00:52, "Stephen Frost" <sfrost@snowman.net>:

   Greetings,

   * Evgeniy Efimkin (efimkin@yandex-team.ru) wrote:

    As a first step I suggest we allow CREATE SUBSCRIPTION for table owner only.

   That's a nice idea but seems like we would want to have a way to filter
   what tables a subscription follows then..? Just failing if the
   publication publishes tables that we don't have access to or are not the
   owner of seems like a poor solution..

   Thanks!

   Stephen

  --------
  Ефимкин Евгений

 --------
 Ефимкин Евгений

--------
Ефимкин Евгений

--------
Ефимкин Евгений

Hi, Evgeniy!

Thanks for working on the feature.

28 нояб. 2018 г., в 21:41, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):

Hello!
I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now old and new tests pass.

22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

Hello!
New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` for own tables.

I've looked into the patch. The code looks good and coherent to nearby code.
There are no docs, obviously, there is WiP.

I've got few questions:
1. How will the subscription work for inherited tables? Do we need tests for that?
2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that?
3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal is embedded into translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändare för att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insert FOR ALL TABLES?
4. How does default behavior differ from FOR ALL TABLES?
5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription?

Best regards, Andrey Borodin.

Hello!

Thank you for questions!

I've got few questions:
1. How will the subscription work for inherited tables? Do we need tests for that?

For subscription created with `FOR TABLE` we can't support inherit tables because subscriber don't know anything about inherit. In new patch i remove `ONLY` for `FOR TABLE` in subscription related statements

2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that?

Added it in new patch

3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal is embedded into translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändare för att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insert FOR ALL TABLES?

I add hint `Use CREATE SUBSCRIPTION ... FOR TABLE ...`

4. How does default behavior differ from FOR ALL TABLES?

The same with default implementation

5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription?

For subscriptions created with `FOR ALL TABLES` (default), you can't change subscribed tables by `ALTER SUBSCRIPTION ADD/DROP` table, you should use `ALTER SUBSCRIPTION REFRESH PUBLICATION`

And i don't know how do export for user created subscriptions, because now non superuser can't select subconninfo column

03.12.2018, 09:06, "Andrey Borodin" <x4mmm@yandex-team.ru>:

Hi, Evgeniy!

Thanks for working on the feature.

 28 нояб. 2018 г., в 21:41, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):

 Hello!
 I wrote some tests(it's just 01_rep_changes.pl but for non superuser) and fix `DROP TABLE` from subscription. Now old and new tests pass.

 22.11.2018, 16:23, "Evgeniy Efimkin" <efimkin@yandex-team.ru>:

 Hello!
 New draft attached with filtering table in subscription (ADD/DROP) and allow non-superusers use` CREATE SUBSCRIPTION` for own tables.

I've looked into the patch. The code looks good and coherent to nearby code.
There are no docs, obviously, there is WiP.

I've got few questions:
1. How will the subscription work for inherited tables? Do we need tests for that?
2. ALTER PUBLICATION has ADD\DROP and SET. Should we add SET too? Or is there a reason not to do that?
3. Message "Must be superuser to create FOR ALL TABLES subscriptions" seems a bit strange to me. Also, this literal is embedded into translations. I do not know how we deal with it, how do we deal for example with "måste vara superanvändare för att skapa prenumerationer" or "для создания подписок нужно быть суперпользователем"? Where do we insert FOR ALL TABLES?
4. How does default behavior differ from FOR ALL TABLES?
5. Can we alter subscription FOR ALL TABLES? Drop some tables out of the subscription?

Best regards, Andrey Borodin.

--------
Ефимкин Евгений

Hi! Thanks for working on the patch!

6 дек. 2018 г., в 21:47, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):
And i don't know how do export for user created subscriptions, because now non superuser can't select subconninfo column

BTW, docs say

When dumping logical replication subscriptions, pg_dump will generate CREATE SUBSCRIPTION commands that use the NOCONNECT option

But I do not see NOCONNECT anywhere and specifically in CREATE SUBSCRIPTION section. There is only "WITH (connect = false)".
And "with (connect = false)" (as in dumpSubscription() now) dump will be successfully restored.
pg_dump now checks for superuser at getSubscriptions(), I think you should patch this too.

In current form, from my POV, most important issue of this patch is complete lack of doc adjustment. And you can fix "NOCONNECT" thing there too.

Please, avoid top-posting (quoting whole message under your reply), this makes harder to read archives at postgresql.org
And also please send patches with version number to distinguish old and new versions.

Best regards, Andrey Borodin.

Hello!
In latest patch i removed `FOR ALL TABLES` clause and `alltables` parameter, now it's look more simple.
Add new system view pg_user_subscirption to allow non-superuser use pg_dump and select addition column from pg_subscrption
Changes docs.
Thanks!

--------
Ефимкин Евгений

Hi!

27 дек. 2018 г., в 12:54, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):

In latest patch i removed `FOR ALL TABLES` clause and `alltables` parameter, now it's look more simple.
Add new system view pg_user_subscirption to allow non-superuser use pg_dump and select addition column from pg_subscrption
Changes docs.

I've reviewed patch again, here are my notes:

1. In create_subscription.sgml and some others. "All tables listed in clause must be present in publication" I think is better to write "All tables listed in clause must present in the publication". But I'm not a native speaker, just looks that it'd be good if someone proofread docs..

2. New view should be called pg_user_subscription or pg_user_subscriptions? Nearby views are plural e.g. pg_publication_tables.

3. I do not know how will this view get into the db during pg_upgrade. Is it somehow handled?

4. In subscriptioncmds.c :

if (stmt->tables&&!connect)

some spaces needed to make AND readable

5. Same file in CreateSubscription() there's foreach collecting tablesiods. This oids are necessary only in on branch of following

if (stmt->tables)

May be we should refactor this, move tablesiods closer to the place where they are used?

6. In AlterSubscription_set_table() and AlterSubscription_drop_table() palloced memory is not free()'d. Is it OK or is it a leak?

7.

errmsg("table \"%s.%s\" not in preset subscription"

Should it be "table does not present in subscription"?

Besides this, patch looks good to me.

Thanks for working on this!

Best regards, Andrey Borodin.

Hi!
1. done
2. rename to pg_user_subscriptions
3. by pg_dump, i checked upgrade from 10 to 12devel, it's work fine
4. done
5. done
6. I took it from AlterSubscription_refresh, in that function no any free()
7. done

--------
Ефимкин Евгений

Hi!

29 янв. 2019 г., в 14:51, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):

<subscription_v2.patch>

Thanks for the next version.

1. In tests the code for "sub reset_pg_hba" is taken from authentication tests (which is fine), but comments are omitted. Let's take them too?

2. 011_rep_changes_nonsuperuser.pl seems a lot like 001_rep_changes.pl with auth adjustments
I think it is OK, but if you know some clever way to refactor that it would be cool. We could avoid duplication of 200 code line of tests or so.

3. I'm not sure, but I'd add some articles here: "All tables listed in _a_ clause must be present in _the_ publication."
"clauses will remove ? table from ? subscription"

4.

qsort(subrel_local_oids, list_length(subrel_states),

sizeof(Oid), oid_cmp);
is indented two times differently, but I think this will be fixed by pg_indent. There are some other indentation issues.

/* Load the library providing us libpq calls. */
/* Check the connection info string. */
load_file("libpqwalreceiver", false);
walrcv_check_conninfo(stmt->conninfo);

Here 2nd comment jumped a line up. And there are superfluous new line in that code block.

Random newline added after

errmsg("ALTER SUBSCRIPTION ... REFRESH is not allowed for disabled subscriptions")));

5. In regression test we only subtract test (fail for nonsuperuser). Can we add some test there too?

All these are merely cosmetical issues. I believe after addressing these we can switch patch to Ready For Committer.

Best regards, Andrey Borodin.

Hi! Thanks for comments
1. fixed
2. in non-superuser we have to use authorization, and FOR table clause, i don't known how merge both files.
3. fixed
4. fixed and run pgindent
5. add some new cases in regression test
--------
Efimkin Evgeny

Hi!

11 февр. 2019 г., в 16:30, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):
<subscription_v3.patch>

The patch seems good to me but cfbot is not happy. Can you please investigate what's wrong with this build?
https://travis-ci.org/postgresql-cfbot/postgresql/builds/492912868

Also, I'm not sure we should drop this lines from docs:

The subscription apply process will run in the local database with the
privileges of a superuser.

Thanks!

Best regards, Andrey Borodin.

Hi!
Thanks for comments!
I fixed build and return lines about subscription apply process in doc

--------
Efimkin Evgeny

Hi!

14 февр. 2019 г., в 20:04, Evgeniy Efimkin <efimkin@yandex-team.ru> написал(а):

<subscription_v4.patch>

I've made some more iterations looking for ideas how to improve the patch and found non.
Code style, docs, tests, make-check worlds, bit status, everything seems OK. A little bit of copied code from dblink (there is no problem like CVE-2018-10915, or is it?) and copied tests.
I'm inclined to mark the patch as RFC if there is no objections.

May be we could also ask some input from cloud managed PostgreSQL providers, what they think about this patch? This patch, actually, is aimed at easing moving to the cloud DB where user has no superuser privileges.

Best regards, Andrey Borodin.