MIME-Version: 1.0
References: <149ff9264db27cdf724b65709fbbaee4bf316835.camel@j-davis.com>
 <CAExHW5vv-78ixJs3arsuZ+12A4AyqehJnDPhejp0m4FAKG6JZA@mail.gmail.com>
 <830a2bc6cbbb2e6e01c6c0d9f31f320822e10603.camel@j-davis.com>
 <CAExHW5sAo4Gv3AwuxQNGSQ2Z45B8_MZEb-SG-B8D-hLMdrquAg@mail.gmail.com>
 <e531ec798298a21d671babd45849092850b8675e.camel@j-davis.com>
 <ac4042d6-204c-9661-9949-79ab8a1f8604@joeconway.com>
 <CAExHW5unvpDv6yMSmqurHP7Du1PqoJFWVxeK-4YNm5EnoNJiSQ@mail.gmail.com>
 <433d0845248e86c0317d9d396926182cfe157340.camel@j-davis.com>
 <CAExHW5sQVJeCgCGMkum_k4Z4VPaK5GkdAjHW-d+F3MngBZ9qBA@mail.gmail.com>
 <05ae37abb207cd6bf6b126780024692d91402b0b.camel@j-davis.com>
 <93392ffa941ab0d436e19e0ab5d04d0e42c02d3f.camel@j-davis.com>
 <26be917cb07b6aa3ef5dd15f6b59d1b375ece6e8.camel@j-davis.com>
 <CALj2ACXDua2Az15Kj3OZFaRm49G8-faemiEEv8t9GNCcsxv8Hw@mail.gmail.com>
 <55201bd916e748acfc754c8f95880dae8e4e5ed0.camel@j-davis.com>
 <CAExHW5v5poohyfNRVQp3yjVE56ej0dWvVMbysSSVtg21aHT3uQ@mail.gmail.com>
 <e71e029309171eb8b6ca393306f9bc5acfbfdf8f.camel@j-davis.com>
 <CAExHW5vF+cp9efQj=-W+Rhbr9f9ZZqt3XFnP0NB5jKsm=C=5tw@mail.gmail.com>
 <2a89b14a2b1622bffb8b137ca1f9ab7866f2d2b9.camel@j-davis.com>
 <61831790a0a937038f78ce09f8dd4cef7de7456a.camel@j-davis.com>
In-Reply-To: <61831790a0a937038f78ce09f8dd4cef7de7456a.camel@j-davis.com>
From: Ashutosh Bapat <ashutosh.bapat.oss@gmail.com>
Date: Mon, 22 Jan 2024 18:41:07 +0530
Message-ID: 
 <CAExHW5uCzS-VeSYQHTHxFSdQik-f_O892xmzrzm2fuO+ro+otA@mail.gmail.com>
Subject: Re: [17] CREATE SUBSCRIPTION ... SERVER
To: Jeff Davis <pgsql@j-davis.com>
Cc: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
 Joe Conway <mail@joeconway.com>,
	pgsql-hackers@postgresql.org
Content-Type: multipart/mixed; boundary="000000000000c93415060f8890d8"
Archived-At: 
 <https://www.postgresql.org/message-id/CAExHW5uCzS-VeSYQHTHxFSdQik-f_O892xmzrzm2fuO%2Bro%2BotA%40mail.gmail.com>
Precedence: bulk

--000000000000c93415060f8890d8
Content-Type: text/plain; charset="UTF-8"
Content-Transfer-Encoding: quoted-printable

Hi Jeff,

On Tue, Jan 16, 2024 at 7:25=E2=80=AFAM Jeff Davis <pgsql@j-davis.com> wrot=
e:
>
> On Fri, 2024-01-12 at 17:17 -0800, Jeff Davis wrote:
> > I think 0004 needs a bit more work, so I'm leaving it off for now,
> > but
> > I'll bring it back in the next patch set.
>
> Here's the next patch set. 0001 - 0003 are mostly the same with some
> improved error messages and some code fixes. I am looking to start
> committing 0001 - 0003 soon, as they have received some feedback
> already and 0004 isn't required for the earlier patches to be useful.
>

I am reviewing the patches. Here are some random comments.

0002 adds a prefix "regress_" to almost every object that is created
in foreign_data.sql. The commit message doesn't say why it's doing so.
But more importantly, the new tests added are lost in all the other
changes. It will be good to have prefix adding changes into its own
patch explaining the reason. The new tests may stay in 0002.
Interestingly the foreign server created in the new tests doesn't have
"regress_" prefix. Why?

Dummy FDW makes me nervous. The way it's written, it may grow into a
full-fledged postgres_fdw and in the process might acquire the same
concerns that postgres_fdw has today. But I will study the patches and
discussion around it more carefully.

I enhanced the postgres_fdw TAP test to use foreign table. Please see
the attached patch. It works as expected. Of course a follow-on work
will require linking the local table and its replica on the publisher
table so that push down will work on replicated tables. But the
concept at least works with your changes. Thanks for that.

I am not sure we need a full-fledged TAP test for testing
subscription. I wouldn't object to it, but TAP tests are heavy. It
should be possible to write the same test as a SQL test by creating
two databases and switching between them. Do you think it's worth
trying that way?

> 0004 could use more discussion. The purpose is to split the privileges
> of pg_create_subscription into two: pg_create_subscription, and
> pg_create_connection. By separating the privileges, it's possible to
> allow someone to create/manage subscriptions to a predefined set of
> foreign servers (on which they have USAGE privileges) without allowing
> them to write an arbitrary connection string.

Haven't studied this patch yet. Will continue reviewing the patches.

--
Best Wishes,
Ashutosh Bapat

--000000000000c93415060f8890d8
Content-Type: text/plain; charset="US-ASCII"; name="repl_table_test.txt"
Content-Disposition: attachment; filename="repl_table_test.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_lroy4bo70>
X-Attachment-Id: f_lroy4bo70

ZGlmZiAtLWdpdCBhL2NvbnRyaWIvcG9zdGdyZXNfZmR3L3QvMDEwX3N1YnNjcmlwdGlvbi5wbCBi
L2NvbnRyaWIvcG9zdGdyZXNfZmR3L3QvMDEwX3N1YnNjcmlwdGlvbi5wbAppbmRleCBkMWQ4MGQw
Njc5Li4zYWUyYjZkYTRhIDEwMDY0NAotLS0gYS9jb250cmliL3Bvc3RncmVzX2Zkdy90LzAxMF9z
dWJzY3JpcHRpb24ucGwKKysrIGIvY29udHJpYi9wb3N0Z3Jlc19mZHcvdC8wMTBfc3Vic2NyaXB0
aW9uLnBsCkBAIC0yMCw3ICsyMCw3IEBAICRub2RlX3N1YnNjcmliZXItPnN0YXJ0OwogCiAjIENy
ZWF0ZSBzb21lIHByZWV4aXN0aW5nIGNvbnRlbnQgb24gcHVibGlzaGVyCiAkbm9kZV9wdWJsaXNo
ZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLAotCSJDUkVBVEUgVEFCTEUgdGFiX2lucyBBUyBTRUxF
Q1QgZ2VuZXJhdGVfc2VyaWVzKDEsMTAwMikgQVMgYSIpOworCSJDUkVBVEUgVEFCTEUgdGFiX2lu
cyBBUyBTRUxFQ1QgYSwgYSArIDEgYXMgYiBGUk9NIGdlbmVyYXRlX3NlcmllcygxLDEwMDIpIEFT
IGEiKTsKIAogIyBSZXBsaWNhdGUgdGhlIGNoYW5nZXMgd2l0aG91dCBjb2x1bW5zCiAkbm9kZV9w
dWJsaXNoZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLCAiQ1JFQVRFIFRBQkxFIHRhYl9ub19jb2wo
KSIpOwpAQCAtMjksNyArMjksNyBAQCAkbm9kZV9wdWJsaXNoZXItPnNhZmVfcHNxbCgncG9zdGdy
ZXMnLAogCiAjIFNldHVwIHN0cnVjdHVyZSBvbiBzdWJzY3JpYmVyCiAkbm9kZV9zdWJzY3JpYmVy
LT5zYWZlX3BzcWwoJ3Bvc3RncmVzJywgIkNSRUFURSBFWFRFTlNJT04gcG9zdGdyZXNfZmR3Iik7
Ci0kbm9kZV9zdWJzY3JpYmVyLT5zYWZlX3BzcWwoJ3Bvc3RncmVzJywgIkNSRUFURSBUQUJMRSB0
YWJfaW5zIChhIGludCkiKTsKKyRub2RlX3N1YnNjcmliZXItPnNhZmVfcHNxbCgncG9zdGdyZXMn
LCAiQ1JFQVRFIFRBQkxFIHRhYl9pbnMgKGEgaW50LCBiIGludCkiKTsKIAogIyBTZXR1cCBsb2dp
Y2FsIHJlcGxpY2F0aW9uCiBteSAkcHVibGlzaGVyX2Nvbm5zdHIgPSAkbm9kZV9wdWJsaXNoZXIt
PmNvbm5zdHIgLiAnIGRibmFtZT1wb3N0Z3Jlcyc7CkBAIC00NSw2ICs0NSw5IEBAICRub2RlX3N1
YnNjcmliZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLAogCSJDUkVBVEUgVVNFUiBNQVBQSU5HIEZP
UiBQVUJMSUMgU0VSVkVSIHRhcF9zZXJ2ZXIiCiApOwogCiskbm9kZV9zdWJzY3JpYmVyLT5zYWZl
X3BzcWwoJ3Bvc3RncmVzJywKKwkiQ1JFQVRFIEZPUkVJR04gVEFCTEUgZl90YWJfaW5zIChhIGlu
dCwgYiBpbnQpIFNFUlZFUiB0YXBfc2VydmVyIE9QVElPTlModGFibGVfbmFtZSAndGFiX2lucycp
IgorKTsKICRub2RlX3N1YnNjcmliZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLAogCSJDUkVBVEUg
U1VCU0NSSVBUSU9OIHRhcF9zdWIgU0VSVkVSIHRhcF9zZXJ2ZXIgUFVCTElDQVRJT04gdGFwX3B1
YiBXSVRIIChwYXNzd29yZF9yZXF1aXJlZD1mYWxzZSkiCiApOwpAQCAtNTMsMTYgKzU2LDE2IEBA
ICRub2RlX3N1YnNjcmliZXItPnNhZmVfcHNxbCgncG9zdGdyZXMnLAogJG5vZGVfc3Vic2NyaWJl
ci0+d2FpdF9mb3Jfc3Vic2NyaXB0aW9uX3N5bmMoJG5vZGVfcHVibGlzaGVyLCAndGFwX3N1Yicp
OwogCiBteSAkcmVzdWx0ID0KLSAgJG5vZGVfc3Vic2NyaWJlci0+c2FmZV9wc3FsKCdwb3N0Z3Jl
cycsICJTRUxFQ1QgY291bnQoKikgRlJPTSB0YWJfaW5zIik7CisgICRub2RlX3N1YnNjcmliZXIt
PnNhZmVfcHNxbCgncG9zdGdyZXMnLCAiU0VMRUNUIGNvdW50KCopIEZST00gKFNFTEVDVCBmLmIg
PSBsLmIgYXMgbWF0Y2ggRlJPTSB0YWJfaW5zIGwsIGZfdGFiX2lucyBmIFdIRVJFIGwuYSA9IGYu
YSkgV0hFUkUgbWF0Y2giKTsKIGlzKCRyZXN1bHQsIHFxKDEwMDIpLCAnY2hlY2sgaW5pdGlhbCBk
YXRhIHdhcyBjb3BpZWQgdG8gc3Vic2NyaWJlcicpOwogCiAkbm9kZV9wdWJsaXNoZXItPnNhZmVf
cHNxbCgncG9zdGdyZXMnLAotCSJJTlNFUlQgSU5UTyB0YWJfaW5zIFNFTEVDVCBnZW5lcmF0ZV9z
ZXJpZXMoMSw1MCkiKTsKKwkiSU5TRVJUIElOVE8gdGFiX2lucyBTRUxFQ1QgYSwgYSArIDEgRlJP
TSBnZW5lcmF0ZV9zZXJpZXMoMTAwMywxMDUwKSBhIik7CiAKICRub2RlX3B1Ymxpc2hlci0+d2Fp
dF9mb3JfY2F0Y2h1cCgndGFwX3N1YicpOwogCiAkcmVzdWx0ID0KLSAgJG5vZGVfc3Vic2NyaWJl
ci0+c2FmZV9wc3FsKCdwb3N0Z3JlcycsICJTRUxFQ1QgY291bnQoKikgRlJPTSB0YWJfaW5zIik7
Ci1pcygkcmVzdWx0LCBxcSgxMDUyKSwgJ2NoZWNrIGluaXRpYWwgZGF0YSB3YXMgY29waWVkIHRv
IHN1YnNjcmliZXInKTsKKyAgJG5vZGVfc3Vic2NyaWJlci0+c2FmZV9wc3FsKCdwb3N0Z3Jlcycs
ICJTRUxFQ1QgY291bnQoKikgRlJPTSAoU0VMRUNUIGYuYiA9IGwuYiBhcyBtYXRjaCBGUk9NIHRh
Yl9pbnMgbCwgZl90YWJfaW5zIGYgV0hFUkUgbC5hID0gZi5hKSBXSEVSRSBtYXRjaCIpOworaXMo
JHJlc3VsdCwgcXEoMTA1MCksICdjaGVjayBpbml0aWFsIGRhdGEgd2FzIGNvcGllZCB0byBzdWJz
Y3JpYmVyJyk7CiAKIGRvbmVfdGVzdGluZygpOwo=
--000000000000c93415060f8890d8--