Date: Sat, 11 Mar 2023 10:50:06 +0000
To: Alexander Lakhin <exclusion@gmail.com>
From: gkokolatos@pm.me
Cc: Tomas Vondra <tomas.vondra@enterprisedb.com>, "shiy.fnst@fujitsu.com" <shiy.fnst@fujitsu.com>, Michael Paquier <michael@paquier.xyz>, Justin Pryzby <pryzby@telsasoft.com>, "pgsql-hackers@lists.postgresql.org" <pgsql-hackers@lists.postgresql.org>, Rachel Heaton <rachelmheaton@gmail.com>
Subject: Re: Add LZ4 compression in pg_dump
Message-ID: <1aBKUlb8BGVUUTkRJwEmU3ud1NyeHVCoC7bpOKAuRYlHHJ7hRiDVXYgad-wmjx1K3pykd_ctyY4Lqy9xA7JGLXOm1Eul-imdZ_7TVr5LY8U=@pm.me>
In-Reply-To: <33496f7c-3449-1426-d568-63f6bca2ac1f@gmail.com>
References: <7zu6GYFi72jByVk2SEe_nd8r1DHhwA35vSntAE9sAfiMX9Lfk6bwimkwmu7GDgYaEvL27SE4qF1uK4_LHWQM4H-SXOwkMAI-GV8PKGmED_E=@pm.me> <AcPUJxzDanXfXdqYOrrZ0L9J9qcnCblkc5z5x9IgKUm7d89pa6B7y8fhUOb2JGDef8h3VP6Ts7j-BYSMmtn50jVuHAT2nBleGosIdVyhG7Q=@pm.me> <OSZPR01MB631087FC3B53AD8B1E3439F7FDA39@OSZPR01MB6310.jpnprd01.prod.outlook.com> <SnTBJOQtJQf_mnXh6icTznqC05gnGqXKWDwjEA15uIx3zf0cJBwY1aK7ULztKoqlXrGM39lHtEpLGmS48A7585FXo1sO6W2YR5RtiDoWpDU=@pm.me> <535565cd-b69f-c064-a845-15c0fc44db4a@enterprisedb.com> <mYBKRYonRy5OiDDYxcxOOJ5xM9iLnuDsGP92zC7mjw8ew1wiccYZIpTvQEmtgRoAXRhaj91SAKwtFluUtRDZ3r6pRdGYLncWK7rXIQrHhFY=@pm.me> <22cb80ed-ec87-54c7-fd5b-cdcf79766bb1@enterprisedb.com> <8421f53d-c9f7-c02e-3278-9f9522a8e8f3@enterprisedb.com> <33496f7c-3449-1426-d568-63f6bca2ac1f@gmail.com>
Feedback-ID: 6383013:user:proton
MIME-Version: 1.0
Content-Type: multipart/mixed;
 boundary="b1_WWkewiey8KJYLKbMsbtLQxNAITelOFm96bBB5SXbo3M"
Archived-At: <https://www.postgresql.org/message-id/1aBKUlb8BGVUUTkRJwEmU3ud1NyeHVCoC7bpOKAuRYlHHJ7hRiDVXYgad-wmjx1K3pykd_ctyY4Lqy9xA7JGLXOm1Eul-imdZ_7TVr5LY8U%3D%40pm.me>
Precedence: bulk

This is a multi-part message in MIME format.

--b1_WWkewiey8KJYLKbMsbtLQxNAITelOFm96bBB5SXbo3M
Content-Type: text/plain; charset=utf-8
Content-Transfer-Encoding: quoted-printable

------- Original Message -------
On Saturday, March 11th, 2023 at 7:00 AM, Alexander Lakhin <exclusion@gmail=
.com> wrote:

> Hello,
> 23.02.2023 23:24, Tomas Vondra wrote:
>=20
> > On 2/23/23 16:26, Tomas Vondra wrote:
> >=20
> > > Thanks for v30 with the updated commit messages. I've pushed 0001 aft=
er
> > > fixing a comment typo and removing (I think) an unnecessary change in=
 an
> > > error message.
> > >=20
> > > I'll give the buildfarm a bit of time before pushing 0002 and 0003.
> >=20
> > I've now pushed 0002 and 0003, after minor tweaks (a couple typos etc.)=
,
> > and marked the CF entry as committed. Thanks for the patch!
> >=20
> > I wonder how difficult would it be to add the zstd compression, so that
> > we don't have the annoying "unsupported" cases.
>=20
>=20
> With the patch 0003 committed, a single warning -Wtype-limits appeared in=
 the
> master branch:
> $ CPPFLAGS=3D"-Og -Wtype-limits" ./configure --with-lz4 -q && make -s -j8
> compress_lz4.c: In function =E2=80=98LZ4File_gets=E2=80=99:
> compress_lz4.c:492:19: warning: comparison of unsigned expression in =
=E2=80=98< 0=E2=80=99 is
> always false [-Wtype-limits]
> 492 | if (dsize < 0)
> |

Thank you Alexander. Please find attached an attempt to address it.

> (I wonder, is it accidental that there no other places that triggers
> the warning, or some buildfarm animals had this check enabled before?)

I can not answer about the buildfarms. Do you think that adding an explicit
check for this warning in meson would help? I am a bit uncertain as I think
that type-limits are included in extra.

@@ -1748,6 +1748,7 @@ common_warning_flags =3D [
   '-Wshadow=3Dcompatible-local',
   # This was included in -Wall/-Wformat in older GCC versions
   '-Wformat-security',
+  '-Wtype-limits',
 ]

>=20
> It is not a false positive as can be proved by the 002_pg_dump.pl modifie=
d as
> follows:
> - program =3D> $ENV{'LZ4'},
>=20
> + program =3D> 'mv',
>=20
> args =3D> [
>=20
> - '-z', '-f', '--rm',
> "$tempdir/compression_lz4_dir/blobs.toc",
> "$tempdir/compression_lz4_dir/blobs.toc.lz4",
> ],
> },

Correct, it is not a false positive. The existing testing framework provide=
s
limited support for exercising error branches. Especially so when those are
dependent on generated output.=20

> A diagnostic logging added shows:
> LZ4File_gets() after LZ4File_read_internal; dsize: 18446744073709551615
>=20
> and pg_restore fails with:
> error: invalid line in large object TOC file
> ".../src/bin/pg_dump/tmp_check/tmp_test_22ri/compression_lz4_dir/blobs.to=
c": "????"

It is a good thing that the restore fails with bad input. Yet it should
have failed earlier. The attached makes certain it does fail earlier.=20

Cheers,
//Georgios

>=20
> Best regards,
> Alexander
--b1_WWkewiey8KJYLKbMsbtLQxNAITelOFm96bBB5SXbo3M
Content-Type: text/x-patch; name=v1-0001-Respect-return-type-of-LZ4File_read_internal.patch
Content-Transfer-Encoding: base64
Content-Disposition: attachment; filename=v1-0001-Respect-return-type-of-LZ4File_read_internal.patch

RnJvbSBiODBiYjUyZWY2NTQ2YWVlOGM4NDMxZDdjYzEyNmZhNGE3NmI1NDNjIE1vbiBTZXAgMTcg
MDA6MDA6MDAgMjAwMQpGcm9tOiBHZW9yZ2lvcyBLb2tvbGF0b3MgPGdrb2tvbGF0b3NAcG0ubWU+
CkRhdGU6IFNhdCwgMTEgTWFyIDIwMjMgMDk6NTQ6NDAgKzAwMDAKU3ViamVjdDogW1BBVENIIHYx
XSBSZXNwZWN0IHJldHVybiB0eXBlIG9mIExaNEZpbGVfcmVhZF9pbnRlcm5hbAoKVGhlIGZ1bmN0
aW9uIExaNEZpbGVfZ2V0cygpIHdhcyBzdG9yaW5nIHRoZSByZXR1cm4gdmFsdWUgb2YKTFo0Rmls
ZV9yZWFkX2ludGVybmFsIGluIGEgdmFyaWFibGUgb2YgdGhlIHdyb25nIHR5cGUsIGRpc3JlZ2Fy
ZGluZyBzaWduLWVzLgpBcyBhIGNvbnNlcXVlbmNlLCBMWjRGaWxlX2dldHMoKSB3b3VsZCBub3Qg
dGFrZSB0aGUgZXJyb3IgcGF0aCB3aGVuIGl0IHNob3VsZC4KCkluIGFuIGF0dGVtcHQgdG8gaW1w
cm92ZSByZWFkYWJpbGl0eSwgc3BlbGwgb3V0IHRoZSBzaWduaWZpY2FuY2Ugb2YgYSBuZWdhdGl2
ZQpyZXR1cm4gdmFsdWUgb2YgTFo0RmlsZV9yZWFkX2ludGVybmFsKCkgaW4gTFo0RmlsZV9yZWFk
KCkuCgpSZXBvcnRlZC1ieTogQWxleGFuZGVyIExha2hpbiA8ZXhjbHVzaW9uQGdtYWlsLmNvbT4K
LS0tCiBzcmMvYmluL3BnX2R1bXAvY29tcHJlc3NfbHo0LmMgfCAxMCArKysrKy0tLS0tCiAxIGZp
bGUgY2hhbmdlZCwgNSBpbnNlcnRpb25zKCspLCA1IGRlbGV0aW9ucygtKQoKZGlmZiAtLWdpdCBh
L3NyYy9iaW4vcGdfZHVtcC9jb21wcmVzc19sejQuYyBiL3NyYy9iaW4vcGdfZHVtcC9jb21wcmVz
c19sejQuYwppbmRleCA2M2U3OTRjZGM2Li5jYzAzOWYwYjQ3IDEwMDY0NAotLS0gYS9zcmMvYmlu
L3BnX2R1bXAvY29tcHJlc3NfbHo0LmMKKysrIGIvc3JjL2Jpbi9wZ19kdW1wL2NvbXByZXNzX2x6
NC5jCkBAIC00NTMsNyArNDUzLDcgQEAgTFo0RmlsZV9yZWFkKHZvaWQgKnB0ciwgc2l6ZV90IHNp
emUsIENvbXByZXNzRmlsZUhhbmRsZSAqQ0ZIKQogCWludAkJCXJldDsKIAogCXJldCA9IExaNEZp
bGVfcmVhZF9pbnRlcm5hbChmcywgcHRyLCBzaXplLCBmYWxzZSk7Ci0JaWYgKHJldCAhPSBzaXpl
ICYmICFMWjRGaWxlX2VvZihDRkgpKQorCWlmIChyZXQgPCAwIHx8IChyZXQgIT0gc2l6ZSAmJiAh
TFo0RmlsZV9lb2YoQ0ZIKSkpCiAJCXBnX2ZhdGFsKCJjb3VsZCBub3QgcmVhZCBmcm9tIGlucHV0
IGZpbGU6ICVzIiwgTFo0RmlsZV9nZXRfZXJyb3IoQ0ZIKSk7CiAKIAlyZXR1cm4gcmV0OwpAQCAt
NDg2LDE0ICs0ODYsMTQgQEAgc3RhdGljIGNoYXIgKgogTFo0RmlsZV9nZXRzKGNoYXIgKnB0ciwg
aW50IHNpemUsIENvbXByZXNzRmlsZUhhbmRsZSAqQ0ZIKQogewogCUxaNEZpbGUgICAgKmZzID0g
KExaNEZpbGUgKikgQ0ZILT5wcml2YXRlX2RhdGE7Ci0Jc2l6ZV90CQlkc2l6ZTsKKwlpbnQJCQly
ZXQ7CiAKLQlkc2l6ZSA9IExaNEZpbGVfcmVhZF9pbnRlcm5hbChmcywgcHRyLCBzaXplLCB0cnVl
KTsKLQlpZiAoZHNpemUgPCAwKQorCXJldCA9IExaNEZpbGVfcmVhZF9pbnRlcm5hbChmcywgcHRy
LCBzaXplLCB0cnVlKTsKKwlpZiAocmV0IDwgMCkKIAkJcGdfZmF0YWwoImNvdWxkIG5vdCByZWFk
IGZyb20gaW5wdXQgZmlsZTogJXMiLCBMWjRGaWxlX2dldF9lcnJvcihDRkgpKTsKIAogCS8qIERv
bmUgcmVhZGluZyAqLwotCWlmIChkc2l6ZSA9PSAwKQorCWlmIChyZXQgPT0gMCkKIAkJcmV0dXJu
IE5VTEw7CiAKIAlyZXR1cm4gcHRyOwotLSAKMi4zNC4xCgo=

--b1_WWkewiey8KJYLKbMsbtLQxNAITelOFm96bBB5SXbo3M--