Horton

Thread

Commits

  1. Validate ltree siglen GiST option to be int-aligned

  2. Fix custom validators call in build_local_reloptions()

  1. BUG #17847: Unaligned memory access in ltree_gist

    The Post Office <noreply@postgresql.org> — 2023-03-16T18:00:01Z

    The following bug has been logged on the website:

Bug reference:      17847
Logged by:          Alexander Lakhin
Email address:      exclusion@gmail.com
PostgreSQL version: 15.2
Operating system:   Ubuntu 22.04
Description:        

When the following query executed with address sanitizers (and
-fsanitize=alignment):
CREATE EXTENSION ltree;
CREATE TABLE lt (t ltree);
INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
generate_series(1, 200) i;
CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));

An incorrect memory access is detected:
ltree_gist.c:66:12: runtime error: member access within misaligned address
0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment

That line contains:
memcpy(LTG_RNODE(result, siglen), right, VARSIZE(right));

Here the following macros are used:
#define VARSIZE(PTR)                                             
VARSIZE_4B(PTR)
/* VARSIZE_4B() should only be used on known-aligned data */
#define VARSIZE_4B(PTR) \
   ((((varattrib_4b *) (PTR))->va_4byte.va_header >> 2) & 0x3FFFFFFF)

#define LTG_RNODE(x, siglen)    ( LTG_ISNORIGHT(x) ? LTG_LNODE(x, siglen) :
LTG_RENODE(x, siglen) )
#define LTG_LNODE(x, siglen)    ( (ltree*)( ( ((char*)(x))+LTG_HDRSIZE ) + (
LTG_ISALLTRUE(x) ? 0 : (siglen) ) ) )
#define LTG_RENODE(x, siglen)   ( (ltree*)( ((char*)LTG_LNODE(x, siglen)) +
VARSIZE(LTG_LNODE(x, siglen))) )

that can be expanded as:
... + VARSIZE( (ltree*)( ( ((char*)(result))+LTG_HDRSIZE ) + (
LTG_ISALLTRUE(result) ? 0 : (siglen) ) ) )

Even though result and LTG_HDRSIZE are aligned, with an arbitrary siglen
value we get a pointer that is not suitable for VARSIZE_4B.

BTW, d952373a9 made the comment in src/include/utils/memutils.h outdated:
 * ... See VARSIZE_4B() and related macros in
 * postgres.h.  ...

  2. Re: BUG #17847: Unaligned memory access in ltree_gist

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-03-16T19:35:15Z

    PG Bug reporting form <noreply@postgresql.org> writes:
> When the following query executed with address sanitizers (and
> -fsanitize=alignment):
> CREATE EXTENSION ltree;
> CREATE TABLE lt (t ltree);
> INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
> generate_series(1, 200) i;
> CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));

> An incorrect memory access is detected:
> ltree_gist.c:66:12: runtime error: member access within misaligned address
> 0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment

Yeah.  So if you ask me, the problem here is that the option for
user-selectable siglen was added with no thought for the possibility
that there might be undocumented implementation restrictions on the
value.  The code is assuming that siglen is MAXALIGN'd (or at least
int-aligned, I did not look too closely), and there was nothing wrong
with that assumption before.

What I'm inclined to do about this is add a restriction that the siglen
value be a multiple of MAXALIGN.  It doesn't look like the reloption
mechanism has a way to specify that declaratively, but we could probably
get close enough by just making LTREE_GET_SIGLEN throw an error if it's
wrong.  That's not ideal because you could probably get through making
an empty index without hitting the error, but I don't offhand see a
way to make it better.

If we decide that we don't need to back-patch a fix for this, maybe
we could instead extend the reloption mechanism to allow stronger
checks on supplied values.  That might be tolerable given how few
alignment-picky machines there are these days.

I wonder which other opclasses besides ltree have the same issue.

			regards, tom lane

  3. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Law <exclusion@gmail.com> — 2023-03-18T07:00:01Z

    16.03.2023 22:35, Tom Lane wrote:
> I wonder which other opclasses besides ltree have the same issue.

I found no other similar places: siglen is also accepted by opclasses
gist__intbig_ops, gist_hstore_ops, gist_trgm_ops, tsvector_ops,
which implementations use functions
_intbig_alloc, ghstore_alloc, gtrgm_alloc, gtsvector_alloc,
and none of them adds siglen to a pointer.
Quick testing confirms that.

(gist__ltree_ops uses ltree_gist_alloc(), but the problematic branch
"if (left) ..." not reached for gist__ltree_ops)

Best regards,
Alexander

  4. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-03-19T17:08:28Z

    On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> PG Bug reporting form <noreply@postgresql.org> writes:
> > When the following query executed with address sanitizers (and
> > -fsanitize=alignment):
> > CREATE EXTENSION ltree;
> > CREATE TABLE lt (t ltree);
> > INSERT INTO lt SELECT format('%s.%s', i / 10, i % 10)::ltree FROM
> > generate_series(1, 200) i;
> > CREATE INDEX ltidx ON lt USING gist (t gist_ltree_ops(siglen=99));
>
> > An incorrect memory access is detected:
> > ltree_gist.c:66:12: runtime error: member access within misaligned address
> > 0x62500019bfd3 for type 'varattrib_4b', which requires 4 byte alignment
>
> Yeah.  So if you ask me, the problem here is that the option for
> user-selectable siglen was added with no thought for the possibility
> that there might be undocumented implementation restrictions on the
> value.  The code is assuming that siglen is MAXALIGN'd (or at least
> int-aligned, I did not look too closely), and there was nothing wrong
> with that assumption before.
>
> What I'm inclined to do about this is add a restriction that the siglen
> value be a multiple of MAXALIGN.  It doesn't look like the reloption
> mechanism has a way to specify that declaratively, but we could probably
> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> wrong.  That's not ideal because you could probably get through making
> an empty index without hitting the error, but I don't offhand see a
> way to make it better.

Sorry for missing this.

Please, note that there are infrastructure of reltoption validators.
I think this is the most appropriate place to check for alignment of
siglen.  That works even for empty indexes.  See the attached patch.

------
Regards,
Alexander Korotkov

  5. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-03-19T17:09:27Z

    On Sat, Mar 18, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> 16.03.2023 22:35, Tom Lane wrote:
>
> I wonder which other opclasses besides ltree have the same issue.
>
>
> I found no other similar places: siglen is also accepted by opclasses
> gist__intbig_ops, gist_hstore_ops, gist_trgm_ops, tsvector_ops,
> which implementations use functions
> _intbig_alloc, ghstore_alloc, gtrgm_alloc, gtsvector_alloc,
> and none of them adds siglen to a pointer.
> Quick testing confirms that.
>
> (gist__ltree_ops uses ltree_gist_alloc(), but the problematic branch
> "if (left) ..." not reached for gist__ltree_ops)

Yep, ltree seems the only place storing aligned value *after*
signature.  Thank you for validating this.

------
Regards,
Alexander Korotkov

  6. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Law <exclusion@gmail.com> — 2023-03-20T07:00:00Z

    Hi,
19.03.2023 20:08, Alexander Korotkov wrote:
> On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> What I'm inclined to do about this is add a restriction that the siglen
>> value be a multiple of MAXALIGN.  It doesn't look like the reloption
>> mechanism has a way to specify that declaratively, but we could probably
>> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
>> wrong.  That's not ideal because you could probably get through making
>> an empty index without hitting the error, but I don't offhand see a
>> way to make it better.
> Sorry for missing this.
>
> Please, note that there are infrastructure of reltoption validators.
> I think this is the most appropriate place to check for alignment of
> siglen.  That works even for empty indexes.  See the attached patch.

Thanks for the fix! It works for me.

Maybe it's worth to reflect this restriction in the documentation too?
      <literal>gist_ltree_ops</literal> GiST opclass approximates a set of
      path labels as a bitmap signature.  Its optional integer parameter
      <literal>siglen</literal> determines the
      signature length in bytes.  The default signature length is 8 bytes.
      Valid values of signature length are between 1 and 2024 bytes.

How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
(There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)

Also maybe change the error message a little:
s/siglen value must be integer-alignment/siglen value must be integer-aligned/
or "int-aligned"? (this spelling can be found in src/)

(There is also a detail message, that probably should be corrected too:
DETAIL:  Valid values are between "1" and "2024".
->
DETAIL:  Valid values are int-aligned positive integers less than 2024.
?)

Best regards,
Alexander

  7. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-18T10:16:00Z

    Hi  Alexander,

Thank you for your feedback.

The revised patch is attached.

On Mon, Mar 20, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> 19.03.2023 20:08, Alexander Korotkov wrote:
> > On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> What I'm inclined to do about this is add a restriction that the siglen
> >> value be a multiple of MAXALIGN.  It doesn't look like the reloption
> >> mechanism has a way to specify that declaratively, but we could probably
> >> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> >> wrong.  That's not ideal because you could probably get through making
> >> an empty index without hitting the error, but I don't offhand see a
> >> way to make it better.
> > Sorry for missing this.
> >
> > Please, note that there are infrastructure of reltoption validators.
> > I think this is the most appropriate place to check for alignment of
> > siglen.  That works even for empty indexes.  See the attached patch.
>
> Thanks for the fix! It works for me.
>
> Maybe it's worth to reflect this restriction in the documentation too?
>       <literal>gist_ltree_ops</literal> GiST opclass approximates a set of
>       path labels as a bitmap signature.  Its optional integer parameter
>       <literal>siglen</literal> determines the
>       signature length in bytes.  The default signature length is 8 bytes.
>       Valid values of signature length are between 1 and 2024 bytes.
>
> How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
> (There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)

I think it's a bit contradictory to say that int alignment is 4 bytes
on most machines, but the minimum value is exactly 4.  The revised
patch says just that length is positive up to 2024.

> Also maybe change the error message a little:
> s/siglen value must be integer-alignment/siglen value must be integer-aligned/
> or "int-aligned"? (this spelling can be found in src/)

Thank you, accepted.

> (There is also a detail message, that probably should be corrected too:
> DETAIL:  Valid values are between "1" and "2024".
> ->
> DETAIL:  Valid values are int-aligned positive integers less than 2024.
> ?)

I can't edit directly the detail message for GUC min/max violation.
But I've corrected the min value to INTALIGN(1).  Also, I've added
detail message for alignment validation.

I'm going to push this if no objections.

------
Regards,
Alexander Korotkov

  8. Re: BUG #17847: Unaligned memory access in ltree_gist

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-18T10:34:39Z

    Hi, Alexander and Alexander!

On Tue, 18 Apr 2023 at 14:16, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi  Alexander,
>
> Thank you for your feedback.
>
> The revised patch is attached.
>
> On Mon, Mar 20, 2023 at 10:00 AM Alexander Lakhin <exclusion@gmail.com> wrote:
> > 19.03.2023 20:08, Alexander Korotkov wrote:
> > > On Thu, Mar 16, 2023 at 10:35 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > >> What I'm inclined to do about this is add a restriction that the siglen
> > >> value be a multiple of MAXALIGN.  It doesn't look like the reloption
> > >> mechanism has a way to specify that declaratively, but we could probably
> > >> get close enough by just making LTREE_GET_SIGLEN throw an error if it's
> > >> wrong.  That's not ideal because you could probably get through making
> > >> an empty index without hitting the error, but I don't offhand see a
> > >> way to make it better.
> > > Sorry for missing this.
> > >
> > > Please, note that there are infrastructure of reltoption validators.
> > > I think this is the most appropriate place to check for alignment of
> > > siglen.  That works even for empty indexes.  See the attached patch.
> >
> > Thanks for the fix! It works for me.
> >
> > Maybe it's worth to reflect this restriction in the documentation too?
> >       <literal>gist_ltree_ops</literal> GiST opclass approximates a set of
> >       path labels as a bitmap signature.  Its optional integer parameter
> >       <literal>siglen</literal> determines the
> >       signature length in bytes.  The default signature length is 8 bytes.
> >       Valid values of signature length are between 1 and 2024 bytes.
> >
> > How about "The length must be a multiple of <type>int</type> alignment between 4 and 2024."?
> > (There is a wording "<type>int</type> alignment (4 bytes on most machines)" in catalogs.sgml.)
>
> I think it's a bit contradictory to say that int alignment is 4 bytes
> on most machines, but the minimum value is exactly 4.  The revised
> patch says just that length is positive up to 2024.
>
> > Also maybe change the error message a little:
> > s/siglen value must be integer-alignment/siglen value must be integer-aligned/
> > or "int-aligned"? (this spelling can be found in src/)
>
> Thank you, accepted.
>
> > (There is also a detail message, that probably should be corrected too:
> > DETAIL:  Valid values are between "1" and "2024".
> > ->
> > DETAIL:  Valid values are int-aligned positive integers less than 2024.
> > ?)
>
> I can't edit directly the detail message for GUC min/max violation.
> But I've corrected the min value to INTALIGN(1).  Also, I've added
> detail message for alignment validation.
>
> I'm going to push this if no objections.

I've looked into the patch v2 and there is a difference in DETAIL text
for the cases:

(1)
 create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
+ERROR:  siglen value must be integer-aligned
+DETAIL:  Valid values are int-aligned positive integers up to 2024.

(2)
+create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
+ERROR:  value 2028 out of bounds for option "siglen"
+DETAIL:  Valid values are between "4" and "2024"

Could we stick to the DETAIL like in (2) for both cases?
Overall the patch seems good to be committed.

Regards,
Pavel Borisov

  9. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-18T10:57:09Z

    Hi, Pavel!

On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> I've looked into the patch v2 and there is a difference in DETAIL text
> for the cases:
>
> (1)
>  create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> +ERROR:  siglen value must be integer-aligned
> +DETAIL:  Valid values are int-aligned positive integers up to 2024.
>
> (2)
> +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> +ERROR:  value 2028 out of bounds for option "siglen"
> +DETAIL:  Valid values are between "4" and "2024"
>
> Could we stick to the DETAIL like in (2) for both cases?

Within ltree we don't have control over error messages, which GUC code
emits about min/max boundaries violation (for sure, we're not going to
patch GUC code in this fix).  So the only thing I can do to match
these two DETAIL is to make both of them 'Valid values are between "4"
and "2024"'.  However, this message would be kind of irrelevant for
"siglen value must be integer-aligned" error.  It's strange for me
when an error mentions alignment, but DETAIL does not.

Do you think we can just remove the DETAIL for "siglen value must be
integer-aligned" error?

------
Regards,
Alexander Korotkov

  10. Re: BUG #17847: Unaligned memory access in ltree_gist

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-18T11:11:15Z

    Hi, Alexander!

On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> Hi, Pavel!
>
> On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > I've looked into the patch v2 and there is a difference in DETAIL text
> > for the cases:
> >
> > (1)
> >  create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > +ERROR:  siglen value must be integer-aligned
> > +DETAIL:  Valid values are int-aligned positive integers up to 2024.
> >
> > (2)
> > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > +ERROR:  value 2028 out of bounds for option "siglen"
> > +DETAIL:  Valid values are between "4" and "2024"
> >
> > Could we stick to the DETAIL like in (2) for both cases?
>
> Within ltree we don't have control over error messages, which GUC code
> emits about min/max boundaries violation (for sure, we're not going to
> patch GUC code in this fix).  So the only thing I can do to match
> these two DETAIL is to make both of them 'Valid values are between "4"
> and "2024"'.  However, this message would be kind of irrelevant for
> "siglen value must be integer-aligned" error.  It's strange for me
> when an error mentions alignment, but DETAIL does not.
>
> Do you think we can just remove the DETAIL for "siglen value must be
> integer-aligned" error?

I'd just propose something like making DETAIL output in ltree look
similar to GUC validation (patch v3). But it's minor and could be done
by removing DETAIL at all or otherwise.

Regards,
Pavel.

  11. Re: BUG #17847: Unaligned memory access in ltree_gist

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-18T11:16:19Z

    On Tue, 18 Apr 2023 at 15:11, Pavel Borisov <pashkin.elfe@gmail.com> wrote:
>
> Hi, Alexander!
>
> On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > Hi, Pavel!
> >
> > On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > > I've looked into the patch v2 and there is a difference in DETAIL text
> > > for the cases:
> > >
> > > (1)
> > >  create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > > +ERROR:  siglen value must be integer-aligned
> > > +DETAIL:  Valid values are int-aligned positive integers up to 2024.
> > >
> > > (2)
> > > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > > +ERROR:  value 2028 out of bounds for option "siglen"
> > > +DETAIL:  Valid values are between "4" and "2024"
> > >
> > > Could we stick to the DETAIL like in (2) for both cases?
> >
> > Within ltree we don't have control over error messages, which GUC code
> > emits about min/max boundaries violation (for sure, we're not going to
> > patch GUC code in this fix).  So the only thing I can do to match
> > these two DETAIL is to make both of them 'Valid values are between "4"
> > and "2024"'.  However, this message would be kind of irrelevant for
> > "siglen value must be integer-aligned" error.  It's strange for me
> > when an error mentions alignment, but DETAIL does not.
> >
> > Do you think we can just remove the DETAIL for "siglen value must be
> > integer-aligned" error?
>
> I'd just propose something like making DETAIL output in ltree look
> similar to GUC validation (patch v3). But it's minor and could be done
> by removing DETAIL at all or otherwise.
I think my confusion on error output is due to alignment being checked
first, then limits. So in DETAIL I see different error reasons for
2025 and 2028.

Pavel

  12. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-18T11:43:38Z

    Hi, Pavel!

On Tue, Apr 18, 2023 at 2:11 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> On Tue, 18 Apr 2023 at 14:57, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Tue, Apr 18, 2023 at 1:34 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> > > I've looked into the patch v2 and there is a difference in DETAIL text
> > > for the cases:
> > >
> > > (1)
> > >  create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2025));
> > > +ERROR:  siglen value must be integer-aligned
> > > +DETAIL:  Valid values are int-aligned positive integers up to 2024.
> > >
> > > (2)
> > > +create index tstidx on ltreetest using gist (t gist_ltree_ops(siglen=2028));
> > > +ERROR:  value 2028 out of bounds for option "siglen"
> > > +DETAIL:  Valid values are between "4" and "2024"
> > >
> > > Could we stick to the DETAIL like in (2) for both cases?
> >
> > Within ltree we don't have control over error messages, which GUC code
> > emits about min/max boundaries violation (for sure, we're not going to
> > patch GUC code in this fix).  So the only thing I can do to match
> > these two DETAIL is to make both of them 'Valid values are between "4"
> > and "2024"'.  However, this message would be kind of irrelevant for
> > "siglen value must be integer-aligned" error.  It's strange for me
> > when an error mentions alignment, but DETAIL does not.
> >
> > Do you think we can just remove the DETAIL for "siglen value must be
> > integer-aligned" error?
>
> I'd just propose something like making DETAIL output in ltree look
> similar to GUC validation (patch v3). But it's minor and could be done
> by removing DETAIL at all or otherwise.

LGMT.  I'm going to push v3 unless there are more comments.

------
Regards,
Alexander Korotkov

  13. Re: BUG #17847: Unaligned memory access in ltree_gist

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-18T15:06:41Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> LGMT.  I'm going to push v3 unless there are more comments.

I think this reads pretty awkwardly:

+ERROR:  siglen value must be integer-aligned
+DETAIL:  Valid are int-aligned values between "4" and "2024".

The DETAIL message's grammar seems a bit off.  Also, this is confusing the
range limitation with the alignment requirement.  How about just saying

ERROR:  siglen value must be a multiple of 4

and leaving out-of-range cases to be handled by the existing check?

			regards, tom lane

  14. Re: BUG #17847: Unaligned memory access in ltree_gist

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-18T15:19:27Z

    Hi, Tom!

On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > LGMT.  I'm going to push v3 unless there are more comments.
>
> I think this reads pretty awkwardly:
>
> +ERROR:  siglen value must be integer-aligned
> +DETAIL:  Valid are int-aligned values between "4" and "2024".
>
> The DETAIL message's grammar seems a bit off.  Also, this is confusing the
> range limitation with the alignment requirement.  How about just saying

> ERROR:  siglen value must be a multiple of 4
I definitely like this wording.

> and leaving out-of-range cases to be handled by the existing check?
But that means that if we try 2025 then we just get it is not multiple
of 4 (and no clue of the range). Then we try 2028 and get another
error that it's outside of range. I suppose giving clues one by one
makes this look like a step-by-step quest. But in principle it's
possible.

Regards,
Pavel Borisov

  15. Re: BUG #17847: Unaligned memory access in ltree_gist

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-18T15:25:42Z

    Pavel Borisov <pashkin.elfe@gmail.com> writes:
> On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> The DETAIL message's grammar seems a bit off.  Also, this is confusing the
>> range limitation with the alignment requirement.  How about just saying
>> ERROR:  siglen value must be a multiple of 4

> I definitely like this wording.

>> and leaving out-of-range cases to be handled by the existing check?

> But that means that if we try 2025 then we just get it is not multiple
> of 4 (and no clue of the range). Then we try 2028 and get another
> error that it's outside of range. I suppose giving clues one by one
> makes this look like a step-by-step quest. But in principle it's
> possible.

Well, that's the case just about everywhere else that there are multiple
constraints on an input.  It'd be impossibly unwieldy, and confusing,
for every error message to include details on all the other errors
you might have hit if you didn't hit that one.

FWIW, I find it odd that the multiple-of-4 error comes out in advance
of the range check; that seems backwards somehow.  Maybe we should
alter the order of applying the range check and the custom validator?

			regards, tom lane

  16. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-19T16:26:15Z

    Hi!

On Tue, Apr 18, 2023 at 6:25 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Pavel Borisov <pashkin.elfe@gmail.com> writes:
> > On Tue, 18 Apr 2023 at 19:06, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> The DETAIL message's grammar seems a bit off.  Also, this is confusing the
> >> range limitation with the alignment requirement.  How about just saying
> >> ERROR:  siglen value must be a multiple of 4

This  message is changed as you proposed.

> > I definitely like this wording.
>
> >> and leaving out-of-range cases to be handled by the existing check?
>
> > But that means that if we try 2025 then we just get it is not multiple
> > of 4 (and no clue of the range). Then we try 2028 and get another
> > error that it's outside of range. I suppose giving clues one by one
> > makes this look like a step-by-step quest. But in principle it's
> > possible.
>
> Well, that's the case just about everywhere else that there are multiple
> constraints on an input.  It'd be impossibly unwieldy, and confusing,
> for every error message to include details on all the other errors
> you might have hit if you didn't hit that one.
>
> FWIW, I find it odd that the multiple-of-4 error comes out in advance
> of the range check; that seems backwards somehow.  Maybe we should
> alter the order of applying the range check and the custom validator?

I think it was intended that custom validators work after builtin
validation, but the current order of validation is caused by bug.  I
don't see why build_local_reloptions() should call custom validators
when validate == false.  Patch 0001 fixes this.  I propose to
backpatch this.

------
Regards,
Alexander Korotkov

  17. Re: BUG #17847: Unaligned memory access in ltree_gist

    Tom Lane <tgl@sss.pgh.pa.us> — 2023-04-19T16:47:15Z

    Alexander Korotkov <aekorotkov@gmail.com> writes:
> [ v4 patches ]

I think you forgot to adjust the expected output in ltree.out.
LGTM otherwise.  I agree with back-patching.

			regards, tom lane

  18. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-19T16:59:34Z

    On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Alexander Korotkov <aekorotkov@gmail.com> writes:
> > [ v4 patches ]
>
> I think you forgot to adjust the expected output in ltree.out.
> LGTM otherwise.  I agree with back-patching.

Thank you, Tom.  The corrected version is attached.
I've registered it on commitfest to make it pass
commitfest.cputube.org.  I'm going to push it once it passes.

------
Regards,
Alexander Korotkov

  19. Re: BUG #17847: Unaligned memory access in ltree_gist

    Pavel Borisov <pashkin.elfe@gmail.com> — 2023-04-19T18:56:52Z

    Hi!

On Wed, 19 Apr 2023 at 20:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > [ v4 patches ]
> >
> > I think you forgot to adjust the expected output in ltree.out.
> > LGTM otherwise.  I agree with back-patching.
>
> Thank you, Tom.  The corrected version is attached.
> I've registered it on commitfest to make it pass
> commitfest.cputube.org.  I'm going to push it once it passes.

I've looked at both patches and they look good. The question about
reporting alignment when the value is out of range went away.
I think they are ready to be pushed.

Regards,
Pavel Borisov

  20. Re: BUG #17847: Unaligned memory access in ltree_gist

    Alexander Korotkov <aekorotkov@gmail.com> — 2023-04-23T11:32:35Z

    On Wed, Apr 19, 2023 at 9:57 PM Pavel Borisov <pashkin.elfe@gmail.com> wrote:
> On Wed, 19 Apr 2023 at 20:59, Alexander Korotkov <aekorotkov@gmail.com> wrote:
> >
> > On Wed, Apr 19, 2023 at 7:47 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > Alexander Korotkov <aekorotkov@gmail.com> writes:
> > > > [ v4 patches ]
> > >
> > > I think you forgot to adjust the expected output in ltree.out.
> > > LGTM otherwise.  I agree with back-patching.
> >
> > Thank you, Tom.  The corrected version is attached.
> > I've registered it on commitfest to make it pass
> > commitfest.cputube.org.  I'm going to push it once it passes.
>
> I've looked at both patches and they look good. The question about
> reporting alignment when the value is out of range went away.
> I think they are ready to be pushed.

Tom, Pavel, thank you!
Pushed!

------
Regards,
Alexander Korotkov