Content-Type: multipart/mixed; boundary="------------S95Ugjzy7t00UJmeRSDX4h4b"
Message-ID: <08cf439a-4566-4698-b60f-d8e9ed348456@pgbackrest.org>
MIME-Version: 1.0
User-Agent: Mozilla Thunderbird
Subject: Re: Improve verification of recovery_target_timeline GUC.
To: Michael Paquier <michael@paquier.xyz>
Cc: Pg Hackers <pgsql-hackers@postgresql.org>
References: <e5d472c7-e9be-4710-8dc4-ebe721b62cea@pgbackrest.org>
 <Z5M2tgQUMgCkyrKJ@paquier.xyz>
 <a17d1bf9-8123-4b86-9c84-5b7526cd70fd@pgbackrest.org>
 <Z67z1UndsbKGOuLt@paquier.xyz>
 <ac0d1e55-75f6-40d1-882d-05274811f294@pgbackrest.org>
 <aArTehpV_Mr8Lqf_@paquier.xyz>
Content-Language: en-US
From: David Steele <david@pgbackrest.org>
In-Reply-To: <aArTehpV_Mr8Lqf_@paquier.xyz>
Date: Fri, 25 Apr 2025 13:50:16 +0000 (UTC)
Archived-At: 
 <https://www.postgresql.org/message-id/08cf439a-4566-4698-b60f-d8e9ed348456%40pgbackrest.org>
Precedence: bulk

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

On 4/24/25 20:12, Michael Paquier wrote:
> On Thu, Apr 24, 2025 at 03:34:29PM +0000, David Steele wrote:
>>
>> Having said that, I think these tests are awfully expensive for a single
>> GUC. Unit tests would definitely be preferable but that's not an option for
>> GUCs, so far as I know.
> 
> On my laptop, 003_recovery_targets.pl increases from 8.2s to 8.7s,
> which seems OK here for the coverage we have.

Multiply half a second by similar tests on all the GUCs and it would add 
up to a lot. But no argument here on keeping the tests.

> "invalid recovery startup fails" is used three times repeatedly for
> the tests with bogus and out-of-bound values.  I'd suggest to make
> these more verbose, with three extras "bogus value", "lower bound
> check" and "upper bound check" added.

I have updated the labels to be more descriptive.

> Side thing noted while reading the area: check_recovery_target_xid()
> does not have any GUC_check_errdetail() giving details for the EINVAL
> and ERANGE cases.  Your addition for recovery_target_timeline is a
> good idea, so perhaps we could do the same there?  No need to do that,
> that's just something I've noticed in passing..

I don't want to add that to this commit but I have added a note to my 
list of PG improvements to make when I have time.

> And you are following the fat-comma convention for the command lines,
> thanks for doing that.  

Just trying to follow the convention from existing tests, but you are 
welcome!

> Note that I'm not planning to do anything here
> for v18 now that we are in feature freeze, these would be for v19 once
> the branch opens.

That was my expectation. I just had some time to get this patch updated 
so took the opportunity.

Regards,
-David
--------------S95Ugjzy7t00UJmeRSDX4h4b
Content-Type: text/plain; charset=UTF-8; name="timeline-check-v3.patch"
Content-Disposition: attachment; filename="timeline-check-v3.patch"
Content-Transfer-Encoding: base64

RnJvbSA1ZThmYTM5MWU3ZDIxNzE0NzVkYzhjODZlZjI5ZmUzYzkzMDVkMmUwIE1vbiBTZXAg
MTcgMDA6MDA6MDAgMjAwMQpGcm9tOiBEYXZpZCBTdGVlbGUgPGRhdmlkQHBnbWFzdGVycy5u
ZXQ+CkRhdGU6IEZyaSwgMjUgQXByIDIwMjUgMTM6Mzc6NDEgKzAwMDAKU3ViamVjdDogSW1w
cm92ZSB2ZXJpZmljYXRpb24gb2YgcmVjb3ZlcnlfdGFyZ2V0X3RpbWVsaW5lIEdVQy4KCkN1
cnJlbnRseSBjaGVja19yZWNvdmVyeV90YXJnZXRfdGltZWxpbmUoKSBjb252ZXJ0cyBhbnkg
dmFsdWUgdGhhdCBpcyBub3QKY3VycmVudCwgbGF0ZXN0LCBvciBhIHZhbGlkIGludGVnZXIg
dG8gMC4gU28gZm9yIGV4YW1wbGU6CgpyZWNvdmVyeV90YXJnZXRfdGltZWxpbmUgPSAnY3Vy
cnJlbnQnCgpyZXN1bHRzIGluIHRoZSBmb2xsb3dpbmcgZXJyb3I6CgpGQVRBTDogIDIyMDIz
OiByZWNvdmVyeSB0YXJnZXQgdGltZWxpbmUgMCBkb2VzIG5vdCBleGlzdAoKU2luY2UgdGhl
cmUgaXMgbm8gcmFuZ2UgY2hlY2tpbmcgZm9yIHVpbnQzMiAoYnV0IHRoZXJlIGlzIGEgY2Fz
dCBmcm9tIHVuc2lnbmVkIGxvbmcpIHRoaXMgc2V0dGluZzoKCnJlY292ZXJ5X3RhcmdldF90
aW1lbGluZSA9ICc5OTk5OTk5OTk5JwoKcmVzdWx0cyBpbiB0aGUgZm9sbG93aW5nIGVycm9y
OgoKRkFUQUw6ICAyMjAyMzogcmVjb3ZlcnkgdGFyZ2V0IHRpbWVsaW5lIDE0MTAwNjU0MDcg
ZG9lcyBub3QgZXhpc3QKCkltcHJvdmUgYnkgYWRkaW5nIGVuZHB0ciBjaGVja2luZyB0byBj
YXRjaCBjb252ZXJzaW9uIGVycm9ycyBhbmQgYWRkIHJhbmdlIGNoZWNraW5nIHRvCmV4Y2x1
ZGUgdmFsdWVzIDwgMiBhbmQgZ3JlYXRlciB0aGFuIFVJTlRfTUFYLgotLS0KIHNyYy9iYWNr
ZW5kL2FjY2Vzcy90cmFuc2FtL3hsb2dyZWNvdmVyeS5jICAgfCAxNyArKysrKy0tCiBzcmMv
dGVzdC9yZWNvdmVyeS90LzAwM19yZWNvdmVyeV90YXJnZXRzLnBsIHwgNTAgKysrKysrKysr
KysrKysrKysrKysrCiAyIGZpbGVzIGNoYW5nZWQsIDY0IGluc2VydGlvbnMoKyksIDMgZGVs
ZXRpb25zKC0pCgpkaWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvYWNjZXNzL3RyYW5zYW0veGxv
Z3JlY292ZXJ5LmMgYi9zcmMvYmFja2VuZC9hY2Nlc3MvdHJhbnNhbS94bG9ncmVjb3Zlcnku
YwppbmRleCA2Y2U5NzlmMmQ4Yi4uZTgyYWE3MzlkNzkgMTAwNjQ0Ci0tLSBhL3NyYy9iYWNr
ZW5kL2FjY2Vzcy90cmFuc2FtL3hsb2dyZWNvdmVyeS5jCisrKyBiL3NyYy9iYWNrZW5kL2Fj
Y2Vzcy90cmFuc2FtL3hsb2dyZWNvdmVyeS5jCkBAIC00OTk0LDEzICs0OTk0LDI0IEBAIGNo
ZWNrX3JlY292ZXJ5X3RhcmdldF90aW1lbGluZShjaGFyICoqbmV3dmFsLCB2b2lkICoqZXh0
cmEsIEd1Y1NvdXJjZSBzb3VyY2UpCiAJCXJ0dGcgPSBSRUNPVkVSWV9UQVJHRVRfVElNRUxJ
TkVfTEFURVNUOwogCWVsc2UKIAl7CisJCWNoYXIJCSAqZW5kcDsKKwkJdWludDY0CQl0aW1l
bGluZTsKIAkJcnR0ZyA9IFJFQ09WRVJZX1RBUkdFVF9USU1FTElORV9OVU1FUklDOwogCiAJ
CWVycm5vID0gMDsKLQkJc3RydG91bCgqbmV3dmFsLCBOVUxMLCAwKTsKLQkJaWYgKGVycm5v
ID09IEVJTlZBTCB8fCBlcnJubyA9PSBFUkFOR0UpCisJCXRpbWVsaW5lID0gc3RydG91NjQo
Km5ld3ZhbCwgJmVuZHAsIDApOworCisJCWlmICgqZW5kcCAhPSAnXDAnIHx8IGVycm5vID09
IEVJTlZBTCB8fCBlcnJubyA9PSBFUkFOR0UpCisJCXsKKwkJCUdVQ19jaGVja19lcnJkZXRh
aWwoIlwiJXNcIiBpcyBub3QgYSB2YWxpZCBudW1iZXIuIiwKKwkJCQkJCQkJInJlY292ZXJ5
X3RhcmdldF90aW1lbGluZSIpOworCQkJcmV0dXJuIGZhbHNlOworCQl9CisKKwkJaWYgKHRp
bWVsaW5lIDwgMSB8fCB0aW1lbGluZSA+IFVJTlRfTUFYKQogCQl7Ci0JCQlHVUNfY2hlY2tf
ZXJyZGV0YWlsKCJcInJlY292ZXJ5X3RhcmdldF90aW1lbGluZVwiIGlzIG5vdCBhIHZhbGlk
IG51bWJlci4iKTsKKwkJCUdVQ19jaGVja19lcnJkZXRhaWwoIlwiJXNcIiBtdXN0IGJlIGJl
dHdlZW4gJXUgYW5kICV1LiIsCisJCQkJCQkJCSJyZWNvdmVyeV90YXJnZXRfdGltZWxpbmUi
LCAxLCBVSU5UX01BWCk7CiAJCQlyZXR1cm4gZmFsc2U7CiAJCX0KIAl9CmRpZmYgLS1naXQg
YS9zcmMvdGVzdC9yZWNvdmVyeS90LzAwM19yZWNvdmVyeV90YXJnZXRzLnBsIGIvc3JjL3Rl
c3QvcmVjb3ZlcnkvdC8wMDNfcmVjb3ZlcnlfdGFyZ2V0cy5wbAppbmRleCAwYWUyZTk4Mjcy
Ny4uYTk2M2Y0NmI3YmIgMTAwNjQ0Ci0tLSBhL3NyYy90ZXN0L3JlY292ZXJ5L3QvMDAzX3Jl
Y292ZXJ5X3RhcmdldHMucGwKKysrIGIvc3JjL3Rlc3QvcmVjb3ZlcnkvdC8wMDNfcmVjb3Zl
cnlfdGFyZ2V0cy5wbApAQCAtMTg3LDQgKzE4Nyw1NCBAQCBvayggJGxvZ2ZpbGUgPX4KIAkg
IHFyL0ZBVEFMOiAuKiByZWNvdmVyeSBlbmRlZCBiZWZvcmUgY29uZmlndXJlZCByZWNvdmVy
eSB0YXJnZXQgd2FzIHJlYWNoZWQvLAogCSdyZWNvdmVyeSBlbmQgYmVmb3JlIHRhcmdldCBy
ZWFjaGVkIGlzIGEgZmF0YWwgZXJyb3InKTsKIAorIyBJbnZhbGlkIHRpbWVsaW5lIHRhcmdl
dAorJG5vZGVfc3RhbmRieSA9IFBvc3RncmVTUUw6OlRlc3Q6OkNsdXN0ZXItPm5ldygnc3Rh
bmRieV85Jyk7Ciskbm9kZV9zdGFuZGJ5LT5pbml0X2Zyb21fYmFja3VwKCRub2RlX3ByaW1h
cnksICdteV9iYWNrdXAnLAorCWhhc19yZXN0b3JpbmcgPT4gMSk7Ciskbm9kZV9zdGFuZGJ5
LT5hcHBlbmRfY29uZigKKwkncG9zdGdyZXNxbC5jb25mJywgInJlY292ZXJ5X3RhcmdldF90
aW1lbGluZSA9ICdib2d1cyciKTsKKworJHJlcyA9IHJ1bl9sb2coCisJWworCQkncGdfY3Rs
JywKKwkJJy0tcGdkYXRhJyA9PiAkbm9kZV9zdGFuZGJ5LT5kYXRhX2RpciwKKwkJJy0tbG9n
JyA9PiAkbm9kZV9zdGFuZGJ5LT5sb2dmaWxlLAorCQknc3RhcnQnLAorCV0pOworb2soISRy
ZXMsICdpbnZhbGlkIHRpbWVsaW5lIHRhcmdldCAoYm9ndXMgdmFsdWUpJyk7CisKK215ICRs
b2dfc3RhcnQgPSAkbm9kZV9zdGFuZGJ5LT53YWl0X2Zvcl9sb2coImlzIG5vdCBhIHZhbGlk
IG51bWJlciIpOworCisjIFRpbWVsaW5lIHRhcmdldCBvdXQgb2YgbWluIHJhbmdlCiskbm9k
ZV9zdGFuZGJ5LT5hcHBlbmRfY29uZigKKwkncG9zdGdyZXNxbC5jb25mJywgInJlY292ZXJ5
X3RhcmdldF90aW1lbGluZSA9ICcwJyIpOworCiskcmVzID0gcnVuX2xvZygKKwlbCisJCSdw
Z19jdGwnLAorCQknLS1wZ2RhdGEnID0+ICRub2RlX3N0YW5kYnktPmRhdGFfZGlyLAorCQkn
LS1sb2cnID0+ICRub2RlX3N0YW5kYnktPmxvZ2ZpbGUsCisJCSdzdGFydCcsCisJXSk7Citv
ayghJHJlcywgJ2ludmFsaWQgdGltZWxpbmUgdGFyZ2V0IChsb3dlciBib3VuZCBjaGVjaykn
KTsKKworJGxvZ19zdGFydCA9ICRub2RlX3N0YW5kYnktPndhaXRfZm9yX2xvZygibXVzdCBi
ZSBiZXR3ZWVuIDEgYW5kIDQyOTQ5NjcyOTUiLAorCQkJCQkJCQkJCSAkbG9nX3N0YXJ0KTsK
KworIyBUaW1lbGluZSB0YXJnZXQgb3V0IG9mIG1heCByYW5nZQorJG5vZGVfc3RhbmRieS0+
YXBwZW5kX2NvbmYoCisJJ3Bvc3RncmVzcWwuY29uZicsICJyZWNvdmVyeV90YXJnZXRfdGlt
ZWxpbmUgPSAnNDI5NDk2NzI5NiciKTsKKworJHJlcyA9IHJ1bl9sb2coCisJWworCQkncGdf
Y3RsJywKKwkJJy0tcGdkYXRhJyA9PiAkbm9kZV9zdGFuZGJ5LT5kYXRhX2RpciwKKwkJJy0t
bG9nJyA9PiAkbm9kZV9zdGFuZGJ5LT5sb2dmaWxlLAorCQknc3RhcnQnLAorCV0pOworb2so
ISRyZXMsICdpbnZhbGlkIHRpbWVsaW5lIHRhcmdldCAodXBwZXIgYm91bmQgY2hlY2spJyk7
CisKKyRsb2dfc3RhcnQgPSAkbm9kZV9zdGFuZGJ5LT53YWl0X2Zvcl9sb2coIm11c3QgYmUg
YmV0d2VlbiAxIGFuZCA0Mjk0OTY3Mjk1IiwKKwkJCQkJCQkJCQkgJGxvZ19zdGFydCk7CisK
IGRvbmVfdGVzdGluZygpOwotLSAKMi4zNC4xCgo=

--------------S95Ugjzy7t00UJmeRSDX4h4b--