Re: BUG #17847: Unaligned memory access in ltree_gist
От | Pavel Borisov |
---|---|
Тема | Re: BUG #17847: Unaligned memory access in ltree_gist |
Дата | |
Msg-id | CALT9ZEF9RnRM9gkE6J_GLN8XTdQ3gZw=c6CHyZY5VueAs4T0PA@mail.gmail.com обсуждение исходный текст |
Ответ на | Re: BUG #17847: Unaligned memory access in ltree_gist (Alexander Korotkov <aekorotkov@gmail.com>) |
Ответы |
Re: BUG #17847: Unaligned memory access in ltree_gist
(Alexander Korotkov <aekorotkov@gmail.com>)
|
Список | pgsql-bugs |
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
В списке pgsql-bugs по дате отправления:
Предыдущее
От: Alexander KorotkovДата:
Сообщение: Re: BUG #17847: Unaligned memory access in ltree_gist
Следующее
От: Alexander KorotkovДата:
Сообщение: Re: BUG #17847: Unaligned memory access in ltree_gist