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-02-10 08:25:19
Message-ID: 896EA252-352C-43A6-977F-24ADF71481CF@yandex-team.ru
Views: Raw Message | Whole Thread | Download mbox
Thread:
Lists: pgsql-hackers

Hi!

> 29 янв. 2019 г., в 14:51, Evgeniy Efimkin <efimkin(at)yandex-team(dot)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.

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Dean Rasheed 2019-02-10 11:18:46 Re: BUG #15623: Inconsistent use of default for updatable view
Previous Message Robert Haas 2019-02-10 06:40:52 Re: dsa_allocate() faliure