Re: [WIP] CREATE SUBSCRIPTION with FOR TABLES clause (table filter)

From: Andrey Borodin <x4mmm(at)yandex-team(dot)ru>
To: Evgeniy Efimkin <efimkin(at)yandex-team(dot)ru>
Cc: Stephen Frost <sfrost(at)snowman(dot)net>, "pgsql-hackers(at)postgresql(dot)org" <pgsql-hackers(at)postgresql(dot)org>, Дмитрий Сарафанников <dsarafan(at)yandex-team(dot)ru>, Владимир Бородин <root(at)simply(dot)name>
Subject: Re: [WIP] CREATE SUBSCRIPTION with FOR TABLES clause (table filter)
Date: 2019-01-14 09:08:10
Message-ID: 60AF8F1F-D84F-420E-A350-8418A4C6DE2D@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi!

> 27 дек. 2018 г., в 12:54, Evgeniy Efimkin <efimkin(at)yandex-team(dot)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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dean Rasheed 2019-01-14 09:09:39 Re: [HACKERS] PATCH: multivariate histograms and MCV lists
Previous Message Andres Freund 2019-01-14 06:39:42 Re: Reducing header interdependencies around heapam.h et al.