MIME-Version: 1.0
References: <CAJ7c6TOy7fUW9MuNeOWor3cSFnQg9tgz=mjXHDb94GORtM_Eyg@mail.gmail.com>
 <ZuIt9-loAPa8tvcY@paquier.xyz> <CAJ7c6TNs8RY4Yvng4m375mAvsS4wHeUcqR8t4b2_i2j=PE6iNQ@mail.gmail.com>
 <Zy2RtPwzjdtgjLnE@paquier.xyz>
In-Reply-To: <Zy2RtPwzjdtgjLnE@paquier.xyz>
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Tue, 12 Nov 2024 17:15:52 +0300
Message-ID: <CAJ7c6TMq46VGLtMVCev2z0QUfUYO1Fnfug0ouLuEZby2sa0+FQ@mail.gmail.com>
Subject: Re: [PATCH] Refactor SLRU to always use long file names
To: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>
Cc: Michael Paquier <michael@paquier.xyz>
Content-Type: multipart/mixed; boundary="00000000000077cd650626b7db9c"
Archived-At: <https://www.postgresql.org/message-id/CAJ7c6TMq46VGLtMVCev2z0QUfUYO1Fnfug0ouLuEZby2sa0%2BFQ%40mail.gmail.com>
Precedence: bulk

--00000000000077cd650626b7db9c
Content-Type: text/plain; charset="UTF-8"

Hi Michael,

Thanks for your feedback!

> Your patch is just doing a rename() of the files from short to long
> names.  How about adding a new TAP script in pg_upgrade that creates a
> couple of empty files with short files names in each path that needs
> to do the transfer?  Then the test could run one pg_upgrade command
> and check that the new names are in place.  You could use a array of
> objects, with the base path, the old name and the new name, then loop
> over it.  With the check in check_slru_segment_filenames() based on
> SLRU_SEG_FILENAMES_CHANGE_CAT_VER, that should work.

OK I gave it a try and discovered that the test becomes very ugly very
fast. Attached is the draft of the test to give you an idea of how
it's going to look like.

In order to trigger renaming of SLRU segments first we have to
downgrade the catalog version in the pg_control file. Otherwise the
check in check_slru_segment_filenames() is not going to pass and
pg_upgrade will do nothing (*). This per se is easily done with
binmode() and pack() however the file has a CRC. Calculating it is not
difficult since we have pg_read_binary_file() and crc32c() SQL
functions, although personally I don't find a need for starting a
cluster for this quite satisfactory. The CRC is stored by the offset
`sizeof(ControlData)-4` and unless I'm wrong is platform-dependent.

I see several solutions for this problem:

* We could add sizeof(ControlData) to the output of `pg_controldata`
so we could use it from Perl
* We could teach `initdb` to override the catalog version
* We could implement a new tool for editing pg_control file

On top of that I should add that the test takes about 7 seconds on my
laptop. Apparently executing two initdb's and one pg_upgrade is not
very cheap. This makes me wonder if instead of writing a new test we
should modify 002_pg_upgrade.pl. This however will make the test even
less readable and maintainable.

What do you think?

(*) BTW I noticed a mistake in the commented code. The condition
should be `>=`, not `<`, i.e:

```
    if(new_cluster.controldata.cat_ver >= SLRU_SEG_FILENAMES_CHANGE_CAT_VER)
        return;
```

-- 
Best regards,
Aleksander Alekseev

--00000000000077cd650626b7db9c
Content-Type: text/plain; charset="US-ASCII"; name="005_slru.pl.txt"
Content-Disposition: attachment; filename="005_slru.pl.txt"
Content-Transfer-Encoding: base64
Content-ID: <f_m3eii3x50>
X-Attachment-Id: f_m3eii3x50

IyBDb3B5cmlnaHQgKGMpIDIwMjQsIFBvc3RncmVTUUwgR2xvYmFsIERldmVsb3BtZW50IEdyb3Vw
Cgp1c2Ugc3RyaWN0Owp1c2Ugd2FybmluZ3MgRkFUQUwgPT4gJ2FsbCc7Cgp1c2UgUG9zdGdyZVNR
TDo6VGVzdDo6Q2x1c3RlcjsKdXNlIFBvc3RncmVTUUw6OlRlc3Q6OlV0aWxzOwp1c2UgVGVzdDo6
TW9yZTsKCiMgVGhpcyB0ZXN0IGVuc3VyZXMgdGhhdCBwZ191cGdyYWRlIHJlbmFtZXMgU0xSVSBz
ZWdtZW50cy4KIyBBZnRlciB0aGUgdXBncmFkZSBhbGwgc2VnbWVudHMgc2hvdWxkIGhhdmUgbG9u
ZyBmaWxlIG5hbWVzLgoKIyBlcXVhbHMgU0xSVV9TRUdfRklMRU5BTUVTX0NIQU5HRV9DQVRfVkVS
IGluIHBnX3VwZ3JhZGUuaApteSAkc2xydV9zZWdfZmlsZW5hbWVzX2NoYW5nZV9jYXRfdmVyID0g
MjAyNDExMTIxOwoKbXkgQHNscnVfZGlycyA9ICgKCSJwZ194YWN0IiwKCSJwZ19jb21taXRfdHMi
LAoJInBnX211bHRpeGFjdC9vZmZzZXRzIiwKCSJwZ19tdWx0aXhhY3QvbWVtYmVycyIsCgkicGdf
c3VidHJhbnMiLAoJInBnX3NlcmlhbCIsCik7CgpteSAkc2hvcnRfc2VnbWVudF9uYW1lID0gIjEy
MzQiOwpteSAkbG9uZ19zZWdtZW50X25hbWUgPSAiMDAwMDAwMDAwMDAxMjM0IjsKCm15ICRvbGRu
b2RlID0gUG9zdGdyZVNRTDo6VGVzdDo6Q2x1c3Rlci0+bmV3KCdvbGRfbm9kZScpOwokb2xkbm9k
ZS0+aW5pdCgpOwpteSAkb2xkYmluZGlyID0gJG9sZG5vZGUtPmNvbmZpZ19kYXRhKCctLWJpbmRp
cicpOwoKbXkgJG5ld25vZGUgPSBQb3N0Z3JlU1FMOjpUZXN0OjpDbHVzdGVyLT5uZXcoJ25ld19u
b2RlJyk7CiRuZXdub2RlLT5pbml0KCk7Cm15ICRuZXdiaW5kaXIgPSAkbmV3bm9kZS0+Y29uZmln
X2RhdGEoJy0tYmluZGlyJyk7CgojIEZpbGwgZGF0YV9kaXIgb2YgdGhlIG9sZCBub2RlIHdpdGgg
U0xSVSBzZWdtZW50cyB0aGF0IHVzZSBzaG9ydCBmaWxlIG5hbWVzLgojIHBnX3VwZ3JhZGUgcmVu
YW1lcyB0aGUgZmlsZXMgd2l0aG91dCBsb29raW5nIGF0IHRoZSBjb250ZW50LCBzbyB0aGUgY29u
dGVudAojIGlzIG5vdCBpbXBvcnRhbnQuCmZvcmVhY2ggbXkgJGRpciAoQHNscnVfZGlycykKewoJ
bXkgJGZuYW1lID0gJG9sZG5vZGUtPmRhdGFfZGlyLiIvIi4kZGlyLiIvIi4kc2hvcnRfc2VnbWVu
dF9uYW1lOwoJb3BlbiBteSAkZmgsICI+IiwgJGZuYW1lIG9yIGRpZSAkITsKCWNsb3NlICRmaDsK
fQoKIyBNb2RpZnkgcGdfY29udHJvbCBvZiB0aGUgb2xkIG5vZGUgdG8gbWFrZSBpdCBsb29rIGxp
a2UgYSB2ZXJzaW9uIHRoYXQgbmVlZHMKIyBtaWdyYXRpb24gKGRlY3JlYXNlIENvbnRyb2xEYXRh
LT5jYXRfdmVyKS4gT3RoZXJ3aXNlIHBnX3VwZ3JhZGUgd2lsbCBza2lwIGl0LgpteSAkcGdfY29u
dHJvbF9mbmFtZSA9ICRvbGRub2RlLT5kYXRhX2Rpci4iL2dsb2JhbC9wZ19jb250cm9sIjsKCm9w
ZW4gbXkgJGZoLCAiKzwiLCAkcGdfY29udHJvbF9mbmFtZSBvciBkaWUgJCE7CmJpbm1vZGUoJGZo
KTsKc3lzc2VlaygkZmgsIDEyLCAwKTsKbXkgJGJpbnZhbCA9IHBhY2soIkwhIiwgJHNscnVfc2Vn
X2ZpbGVuYW1lc19jaGFuZ2VfY2F0X3ZlciAtIDEpOwpzeXN3cml0ZSgkZmgsICRiaW52YWwsIDQp
OwpjbG9zZSgkZmgpOwoKIyBDYWxjdWxhdGUgQ1JDIG9mIHRoZSB1cGRhdGVkIGZpbGUgdXNpbmcg
cGdfcmVhZF9iaW5hcnlfZmlsZSgpIGFuZCBjcmMzMmMoKQpteSAkZnNpemUgPSAtcyAkcGdfY29u
dHJvbF9mbmFtZTsgIyBXUk9ORyEgc2hvdWxkIGJlIHNpemVvZihDb250cm9sRmlsZSkKJG5ld25v
ZGUtPnN0YXJ0OwpteSAkbmV3Y3JjOwokbmV3bm9kZS0+cHNxbCgKCSJwb3N0Z3JlcyIsCgkiU0VM
RUNUIGNyYzMyKHBnX3JlYWRfYmluYXJ5X2ZpbGUoJyIuJHBnX2NvbnRyb2xfZm5hbWUuIicsMCwi
LigkZnNpemUtNCkuIikpOyIsCglzdGRvdXQgPT4gXCRuZXdjcmMsCglvbl9lcnJvcl9kaWUgPT4g
MSwKCSk7CiRuZXdub2RlLT5zdG9wOwoKIyBVcGRhdGUgQ1JDCm9wZW4gJGZoLCAiKzwiLCAkcGdf
Y29udHJvbF9mbmFtZSBvciBkaWUgJCE7CmJpbm1vZGUoJGZoKTsKc3lzc2VlaygkZmgsICRmc2l6
ZS00LCAwKTsKbXkgJGJpbmNyYyA9IHBhY2soIkwhIiwgJG5ld2NyYyk7CnN5c3dyaXRlKCRmaCwg
JGJpbmNyYywgNCk7CmNsb3NlKCRmaCk7CgokbmV3bm9kZS0+Y29tbWFuZF9vaygKCVsKCQkncGdf
dXBncmFkZScsCgkJJy0tb2xkLWRhdGFkaXInLCAkb2xkbm9kZS0+ZGF0YV9kaXIsCgkJJy0tbmV3
LWRhdGFkaXInLCAkbmV3bm9kZS0+ZGF0YV9kaXIsCgkJJy0tb2xkLWJpbmRpcicsICRvbGRiaW5k
aXIsCgkJJy0tbmV3LWJpbmRpcicsICRuZXdiaW5kaXIsCgldLAoJJ3J1biBvZiBwZ191cGdyYWRl
Jyk7CgojIENoZWNrIHRoYXQgcGdfdXBncmFkZSByZW5hbWVkIHRoZSBTTFJVIHNlZ21lbnRzIHdl
IGNyZWF0ZWQKZm9yZWFjaCBteSAkZGlyIChAc2xydV9kaXJzKQp7CglvaygtZSAkbmV3bm9kZS0+
ZGF0YV9kaXIuIi8iLiRkaXIuIi8iLiRsb25nX3NlZ21lbnRfbmFtZSk7Cn0KCmRvbmVfdGVzdGlu
ZygpOwo=
--00000000000077cd650626b7db9c--