Content-Type: multipart/mixed; boundary="------------cD9778ugGAcE000ITP093Fb7"
Message-ID: <3d719e62-64d2-5184-eebf-6128bcb398e2@enterprisedb.com>
Date: Thu, 5 Jan 2023 21:43:42 +0100
MIME-Version: 1.0
User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:102.0) Gecko/20100101
 Thunderbird/102.6.0
Subject: Re: postgres_fdw: using TABLESAMPLE to collect remote sample
From: Tomas Vondra <tomas.vondra@enterprisedb.com>
To: Tom Lane <tgl@sss.pgh.pa.us>
Cc: James Finnerty <jfinnert@amazon.com>, pgsql-hackers@lists.postgresql.org
References: <151e835f-55d6-ddbc-b5b4-07ee606aba44@enterprisedb.com>
 <03182011-10a4-97b4-7c70-d99aa1c11eed@enterprisedb.com>
 <167112207543.1120.12195641719862911275.pgcf@coridan.postgresql.org>
 <3377008.1671122816@sss.pgh.pa.us>
 <cd3f2fda-d4cc-4371-210f-fc339d53b885@enterprisedb.com>
 <6da357b0-fdd3-095d-a06c-87666f4a5a8c@enterprisedb.com>
 <951485.1672461744@sss.pgh.pa.us>
 <233ee0b3-4da0-3370-cd0b-1b8528851ddd@enterprisedb.com>
 <1040922.1672507431@sss.pgh.pa.us>
 <d13d28b9-30d0-27e8-d58e-d3e0c76b3bda@enterprisedb.com>
Content-Language: en-US
In-Reply-To: <d13d28b9-30d0-27e8-d58e-d3e0c76b3bda@enterprisedb.com>
Archived-At: <https://www.postgresql.org/message-id/3d719e62-64d2-5184-eebf-6128bcb398e2%40enterprisedb.com>
Precedence: bulk

This is a multi-part message in MIME format.
--------------cD9778ugGAcE000ITP093Fb7
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 7bit

Hi,

here's two minor postgres_fdw patches, addressing the two issues
discussed last week.


1) 0001-Fix-stale-comment-in-postgres_fdw.patch

The first one cleans up the comment referencing the sample rate
adjustment. While doing that, I realized without the adjustment we
should never get sample_rate outside the valid range because we do:

    if ((reltuples <= 0) || (targrows >= reltuples))
        method = ANALYZE_SAMPLE_OFF;

So I removed the clamping, or rather replaced it with an assert.


2) 0002-WIP-check-relkind-in-FDW-analyze.patch

The second patch adds the relkind check, so that we only issue
TABLESAMPLE on valid relation types (tables, matviews). But I'm not sure
we actually need it - the example presented earlier was foreign table
pointing to a sequence. But that actually works fine even without this
patch, because fore sequences we have reltuples=1, which disables
sampling due to the check mentioned above (because targrows >= 1).

The other issue with this patch is that it seems wrong to check the
relkind value locally - instead we should check this on the remote
server, because that's where we'll run TABLESAMPLE. Currently there are
no differences between versions, but what if we implement TABLESAMPLE
for another relkind in the future? Then we'll either fail to use
sampling or (worse) we'll try using TABLESAMPLE for unsupported relkind,
depending on which of the servers has newer version.


regards

-- 
Tomas Vondra
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--------------cD9778ugGAcE000ITP093Fb7
Content-Type: text/x-patch; charset=UTF-8;
 name="0002-WIP-check-relkind-in-FDW-analyze.patch"
Content-Disposition: attachment;
 filename="0002-WIP-check-relkind-in-FDW-analyze.patch"
Content-Transfer-Encoding: base64

RnJvbSA2ODFiMzU2OTI3OGJjNTViNGUyYmJmY2YxMjkyNDU3MDI2MTZlY2Q5IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBUb21hcyBWb25kcmEgPHRvbWFzLnZvbmRyYUBwb3N0
Z3Jlc3FsLm9yZz4KRGF0ZTogVGh1LCA1IEphbiAyMDIzIDIxOjEzOjIwICswMTAwClN1Ympl
Y3Q6IFtQQVRDSCAyLzJdIFdJUDogY2hlY2sgcmVsa2luZCBpbiBGRFcgYW5hbHl6ZQoKLS0t
CiBjb250cmliL3Bvc3RncmVzX2Zkdy9kZXBhcnNlLmMgICAgICB8ICAyICstCiBjb250cmli
L3Bvc3RncmVzX2Zkdy9wb3N0Z3Jlc19mZHcuYyB8IDE5ICsrKysrKysrKysrKysrKy0tLS0K
IDIgZmlsZXMgY2hhbmdlZCwgMTYgaW5zZXJ0aW9ucygrKSwgNSBkZWxldGlvbnMoLSkKCmRp
ZmYgLS1naXQgYS9jb250cmliL3Bvc3RncmVzX2Zkdy9kZXBhcnNlLmMgYi9jb250cmliL3Bv
c3RncmVzX2Zkdy9kZXBhcnNlLmMKaW5kZXggNDQ2MWZiMDZmMDIuLjlmMzlkNzkyMjgwIDEw
MDY0NAotLS0gYS9jb250cmliL3Bvc3RncmVzX2Zkdy9kZXBhcnNlLmMKKysrIGIvY29udHJp
Yi9wb3N0Z3Jlc19mZHcvZGVwYXJzZS5jCkBAIC0yMzgzLDcgKzIzODMsNyBAQCBkZXBhcnNl
QW5hbHl6ZVR1cGxlc1NxbChTdHJpbmdJbmZvIGJ1ZiwgUmVsYXRpb24gcmVsKQogCWluaXRT
dHJpbmdJbmZvKCZyZWxuYW1lKTsKIAlkZXBhcnNlUmVsYXRpb24oJnJlbG5hbWUsIHJlbCk7
CiAKLQlhcHBlbmRTdHJpbmdJbmZvU3RyaW5nKGJ1ZiwgIlNFTEVDVCByZWx0dXBsZXMgRlJP
TSBwZ19jYXRhbG9nLnBnX2NsYXNzIFdIRVJFIG9pZCA9ICIpOworCWFwcGVuZFN0cmluZ0lu
Zm9TdHJpbmcoYnVmLCAiU0VMRUNUIHJlbHR1cGxlcywgcmVsa2luZCBGUk9NIHBnX2NhdGFs
b2cucGdfY2xhc3MgV0hFUkUgb2lkID0gIik7CiAJZGVwYXJzZVN0cmluZ0xpdGVyYWwoYnVm
LCByZWxuYW1lLmRhdGEpOwogCWFwcGVuZFN0cmluZ0luZm9TdHJpbmcoYnVmLCAiOjpwZ19j
YXRhbG9nLnJlZ2NsYXNzIik7CiB9CmRpZmYgLS1naXQgYS9jb250cmliL3Bvc3RncmVzX2Zk
dy9wb3N0Z3Jlc19mZHcuYyBiL2NvbnRyaWIvcG9zdGdyZXNfZmR3L3Bvc3RncmVzX2Zkdy5j
CmluZGV4IDY1ODIyYjhhOWFhLi41MGIwMzA2NzE1MiAxMDA2NDQKLS0tIGEvY29udHJpYi9w
b3N0Z3Jlc19mZHcvcG9zdGdyZXNfZmR3LmMKKysrIGIvY29udHJpYi9wb3N0Z3Jlc19mZHcv
cG9zdGdyZXNfZmR3LmMKQEAgLTQ5NzgsNyArNDk3OCw3IEBAIHBvc3RncmVzQW5hbHl6ZUZv
cmVpZ25UYWJsZShSZWxhdGlvbiByZWxhdGlvbiwKICAqCQlDb3VudCB0dXBsZXMgaW4gZm9y
ZWlnbiB0YWJsZSAoanVzdCBnZXQgcGdfY2xhc3MucmVsdHVwbGVzKS4KICAqLwogc3RhdGlj
IGRvdWJsZQotcG9zdGdyZXNDb3VudFR1cGxlc0ZvckZvcmVpZ25UYWJsZShSZWxhdGlvbiBy
ZWxhdGlvbikKK3Bvc3RncmVzQ291bnRUdXBsZXNGb3JGb3JlaWduVGFibGUoUmVsYXRpb24g
cmVsYXRpb24sIGJvb2wgKmNhbl9zYW1wbGUpCiB7CiAJRm9yZWlnblRhYmxlICp0YWJsZTsK
IAlVc2VyTWFwcGluZyAqdXNlcjsKQEAgLTQ5ODYsNiArNDk4NiwxMCBAQCBwb3N0Z3Jlc0Nv
dW50VHVwbGVzRm9yRm9yZWlnblRhYmxlKFJlbGF0aW9uIHJlbGF0aW9uKQogCVN0cmluZ0lu
Zm9EYXRhIHNxbDsKIAlQR3Jlc3VsdCAgICp2b2xhdGlsZSByZXMgPSBOVUxMOwogCXZvbGF0
aWxlIGRvdWJsZSByZWx0dXBsZXMgPSAtMTsKKwl2b2xhdGlsZSBjaGFyIHJlbGtpbmQgPSAw
OworCisJLyogYXNzdW1lIHRoZSByZWxraW5kIGNhbid0IGJlIHNhbXBsZWQgKi8KKwkqY2Fu
X3NhbXBsZSA9IGZhbHNlOwogCiAJLyoKIAkgKiBHZXQgdGhlIGNvbm5lY3Rpb24gdG8gdXNl
LiAgV2UgZG8gdGhlIHJlbW90ZSBhY2Nlc3MgYXMgdGhlIHRhYmxlJ3MKQEAgLTUwMDgsOSAr
NTAxMiwxMCBAQCBwb3N0Z3Jlc0NvdW50VHVwbGVzRm9yRm9yZWlnblRhYmxlKFJlbGF0aW9u
IHJlbGF0aW9uKQogCQlpZiAoUFFyZXN1bHRTdGF0dXMocmVzKSAhPSBQR1JFU19UVVBMRVNf
T0spCiAJCQlwZ2Zkd19yZXBvcnRfZXJyb3IoRVJST1IsIHJlcywgY29ubiwgZmFsc2UsIHNx
bC5kYXRhKTsKIAotCQlpZiAoUFFudHVwbGVzKHJlcykgIT0gMSB8fCBQUW5maWVsZHMocmVz
KSAhPSAxKQorCQlpZiAoUFFudHVwbGVzKHJlcykgIT0gMSB8fCBQUW5maWVsZHMocmVzKSAh
PSAyKQogCQkJZWxvZyhFUlJPUiwgInVuZXhwZWN0ZWQgcmVzdWx0IGZyb20gZGVwYXJzZUFu
YWx5emVUdXBsZXNTcWwgcXVlcnkiKTsKIAkJcmVsdHVwbGVzID0gc3RydG9kKFBRZ2V0dmFs
dWUocmVzLCAwLCAwKSwgTlVMTCk7CisJCXJlbGtpbmQgPSAqKFBRZ2V0dmFsdWUocmVzLCAw
LCAxKSk7CiAJfQogCVBHX0ZJTkFMTFkoKTsKIAl7CkBAIC01MDIxLDYgKzUwMjYsMTAgQEAg
cG9zdGdyZXNDb3VudFR1cGxlc0ZvckZvcmVpZ25UYWJsZShSZWxhdGlvbiByZWxhdGlvbikK
IAogCVJlbGVhc2VDb25uZWN0aW9uKGNvbm4pOwogCisJKmNhbl9zYW1wbGUgPSAocmVsa2lu
ZCA9PSBSRUxLSU5EX1JFTEFUSU9OIHx8CisJCQkJICAgcmVsa2luZCA9PSBSRUxLSU5EX01B
VFZJRVcgfHwKKwkJCQkgICByZWxraW5kID09IFJFTEtJTkRfUEFSVElUSU9ORURfVEFCTEUp
OworCiAJcmV0dXJuIHJlbHR1cGxlczsKIH0KIApAQCAtNTE2NywxMyArNTE3NiwxNSBAQCBw
b3N0Z3Jlc0FjcXVpcmVTYW1wbGVSb3dzRnVuYyhSZWxhdGlvbiByZWxhdGlvbiwgaW50IGVs
ZXZlbCwKIAkgKi8KIAlpZiAobWV0aG9kICE9IEFOQUxZWkVfU0FNUExFX09GRikKIAl7Ci0J
CXJlbHR1cGxlcyA9IHBvc3RncmVzQ291bnRUdXBsZXNGb3JGb3JlaWduVGFibGUocmVsYXRp
b24pOworCQlib29sCWNhbl9zYW1wbGU7CisKKwkJcmVsdHVwbGVzID0gcG9zdGdyZXNDb3Vu
dFR1cGxlc0ZvckZvcmVpZ25UYWJsZShyZWxhdGlvbiwgJmNhbl9zYW1wbGUpOwogCiAJCS8q
CiAJCSAqIFJlbW90ZSdzIHJlbHR1cGxlcyBjb3VsZCBiZSAwIG9yIC0xIGlmIHRoZSB0YWJs
ZSBoYXMgbmV2ZXIgYmVlbgogCQkgKiB2YWN1dW1lZC9hbmFseXplZC4gIEluIHRoYXQgY2Fz
ZSwgZGlzYWJsZSBzYW1wbGluZyBhZnRlciBhbGwuCiAJCSAqLwotCQlpZiAoKHJlbHR1cGxl
cyA8PSAwKSB8fCAodGFyZ3Jvd3MgPj0gcmVsdHVwbGVzKSkKKwkJaWYgKCghY2FuX3NhbXBs
ZSkgfHwgKHJlbHR1cGxlcyA8PSAwKSB8fCAodGFyZ3Jvd3MgPj0gcmVsdHVwbGVzKSkKIAkJ
CW1ldGhvZCA9IEFOQUxZWkVfU0FNUExFX09GRjsKIAkJZWxzZQogCQl7Ci0tIAoyLjM5LjAK
Cg==
--------------cD9778ugGAcE000ITP093Fb7
Content-Type: text/x-patch; charset=UTF-8;
 name="0001-Fix-stale-comment-in-postgres_fdw.patch"
Content-Disposition: attachment;
 filename="0001-Fix-stale-comment-in-postgres_fdw.patch"
Content-Transfer-Encoding: base64

RnJvbSAwMjBiYTZhZmI4NzI4N2Q3ODI0NzcwNTQ5NDlkNzc5NTQzNzdkMzQ3IE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBUb21hcyBWb25kcmEgPHRvbWFzLnZvbmRyYUBwb3N0
Z3Jlc3FsLm9yZz4KRGF0ZTogVGh1LCA1IEphbiAyMDIzIDIwOjExOjQ5ICswMTAwClN1Ympl
Y3Q6IFtQQVRDSCAxLzJdIEZpeCBzdGFsZSBjb21tZW50IGluIHBvc3RncmVzX2ZkdwoKQSBj
b21tZW50IHdhcyBsZWZ0IGJlaGluZCByZWZlcmVuY2luZyBhIGJpdCAoc2FtcGxlIHJhdGUg
YWRqdXN0bWVudCkKcmVtb3ZlZCBmcm9tIDhhZDUxYjVmNDQuIFNvIGNsZWFuIHRoYXQgdXAs
IGFuZCB3aGlsZSBhdCBpdCBhbHNvIHJlbW92ZQpjbGFtcGluZyBvZiB0aGUgc2FtcGxlIHJh
dGUgd2hpY2ggc2hvdWxkIG5vdCBiZSBuZWNlc3Nhcnkgd2l0aG91dCB0aGUKY2xhbXBpbmcs
IGFuZCBqdXN0IGNoZWNrIHRoYXQgd2l0aCBhbiBhc3NlcnQuCi0tLQogY29udHJpYi9wb3N0
Z3Jlc19mZHcvcG9zdGdyZXNfZmR3LmMgfCA3ICsrKystLS0KIDEgZmlsZSBjaGFuZ2VkLCA0
IGluc2VydGlvbnMoKyksIDMgZGVsZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvY29udHJpYi9w
b3N0Z3Jlc19mZHcvcG9zdGdyZXNfZmR3LmMgYi9jb250cmliL3Bvc3RncmVzX2Zkdy9wb3N0
Z3Jlc19mZHcuYwppbmRleCAzMzJiNGE1Y2RlYi4uNjU4MjJiOGE5YWEgMTAwNjQ0Ci0tLSBh
L2NvbnRyaWIvcG9zdGdyZXNfZmR3L3Bvc3RncmVzX2Zkdy5jCisrKyBiL2NvbnRyaWIvcG9z
dGdyZXNfZmR3L3Bvc3RncmVzX2Zkdy5jCkBAIC01MjA0LDEwICs1MjA0LDExIEBAIHBvc3Rn
cmVzQWNxdWlyZVNhbXBsZVJvd3NGdW5jKFJlbGF0aW9uIHJlbGF0aW9uLCBpbnQgZWxldmVs
LAogCQkJc2FtcGxlX2ZyYWMgPSB0YXJncm93cyAvIHJlbHR1cGxlczsKIAogCQkJLyoKLQkJ
CSAqIEVuc3VyZSB0aGUgc2FtcGxpbmcgcmF0ZSBpcyBiZXR3ZWVuIDAuMCBhbmQgMS4wLCBl
dmVuIGFmdGVyIHRoZQotCQkJICogMTAlIGFkanVzdG1lbnQgYWJvdmUuICAoQ2xhbXBpbmcg
dG8gMC4wIGlzIGp1c3QgcGFyYW5vaWEuKQorCQkJICogV2Ugc2hvdWxkIG5ldmVyIGdldCBz
YW1wbGluZyByYXRlIG91dHNpZGUgdGhlIHZhbGlkIHJhbmdlCisJCQkgKiAoYmV0d2VlbiAw
LjAgYW5kIDEuMCksIGJlY2F1c2UgdGhvc2UgY2FzZXMgc2hvdWxkIGJlIGNvdmVyZWQKKwkJ
CSAqIGJ5IHRoZSBwcmV2aW91cyBicmFuY2ggdGhhdCBzZXRzIEFOQUxZWkVfU0FNUExFX09G
Ri4KIAkJCSAqLwotCQkJc2FtcGxlX2ZyYWMgPSBNaW4oMS4wLCBNYXgoMC4wLCBzYW1wbGVf
ZnJhYykpOworCQkJQXNzZXJ0KHNhbXBsZV9mcmFjID09IE1pbigxLjAsIE1heCgwLjAsIHNh
bXBsZV9mcmFjKSkpOwogCQl9CiAJfQogCi0tIAoyLjM5LjAKCg==

--------------cD9778ugGAcE000ITP093Fb7--