Horton

Thread

Commits

  1. Check the snapshot argument of index_beginscan and family

  1. [PATCH] Check snapshot argument of index_beginscan and family

    Aleksander Alekseev <aleksander@timescale.com> — 2022-11-28T10:07:42Z

    Hi hackers,

A colleague of mine (cc'ed) reported that he was able to pass a NULL
snapshot to index_beginscan() and it even worked to a certain degree.

I took my toy extension [1] and replaced the argument with NULL as an
experiment:

```
eax=# CREATE EXTENSION experiment;
CREATE EXTENSION
eax=# SELECT phonebook_lookup_index('Alice');
 phonebook_lookup_index
------------------------
                     -1
(1 row)

eax=# SELECT phonebook_insert('Bob', 456);
 phonebook_insert
------------------
                1
(1 row)

eax=# SELECT phonebook_lookup_index('Alice');
 phonebook_lookup_index
------------------------
                     -1
(1 row)

eax=# SELECT phonebook_insert('Alice', 123);
 phonebook_insert
------------------
                2
(1 row)

eax=# SELECT phonebook_lookup_index('Alice');
server closed the connection unexpectedly
    This probably means the server terminated abnormally
    before or while processing the request.
```

So evidently it really works as long as the index doesn't find any
matching rows.

This could be really confusing for the extension authors so here is a
patch that adds corresponding Asserts().

[1]: https://github.com/afiskon/postgresql-extensions/tree/main/005-table-access

-- 
Best regards,
Aleksander Alekseev

  2. Re: [PATCH] Check snapshot argument of index_beginscan and family

    Pavel Borisov <pashkin.elfe@gmail.com> — 2022-11-28T10:23:29Z

    Hi, Alexander!
> A colleague of mine (cc'ed) reported that he was able to pass a NULL
> snapshot to index_beginscan() and it even worked to a certain degree.
>
> I took my toy extension [1] and replaced the argument with NULL as an
> experiment:
>
> ```
> eax=# CREATE EXTENSION experiment;
> CREATE EXTENSION
> eax=# SELECT phonebook_lookup_index('Alice');
>  phonebook_lookup_index
> ------------------------
>                      -1
> (1 row)
>
> eax=# SELECT phonebook_insert('Bob', 456);
>  phonebook_insert
> ------------------
>                 1
> (1 row)
>
> eax=# SELECT phonebook_lookup_index('Alice');
>  phonebook_lookup_index
> ------------------------
>                      -1
> (1 row)
>
> eax=# SELECT phonebook_insert('Alice', 123);
>  phonebook_insert
> ------------------
>                 2
> (1 row)
>
> eax=# SELECT phonebook_lookup_index('Alice');
> server closed the connection unexpectedly
>     This probably means the server terminated abnormally
>     before or while processing the request.
> ```
>
> So evidently it really works as long as the index doesn't find any
> matching rows.
>
> This could be really confusing for the extension authors so here is a
> patch that adds corresponding Asserts().
>
> [1]: https://github.com/afiskon/postgresql-extensions/tree/main/005-table-access
I think it's a nice catch and worth fixing. The one thing I don't
agree with is using asserts for handling the error that can appear
because most probably the server is built with assertions off and in
this case, there still will be a crash in this case. I'd do this with
report ERROR. Otherwise, the patch looks right and worth committing.

Kind regards,
Pavel Borisov.

  3. Re: [PATCH] Check snapshot argument of index_beginscan and family

    Aleksander Alekseev <aleksander@timescale.com> — 2022-11-28T10:29:55Z

    Hi Pavel,

Thanks for the feedback!

> I think it's a nice catch and worth fixing. The one thing I don't
> agree with is using asserts for handling the error that can appear
> because most probably the server is built with assertions off and in
> this case, there still will be a crash in this case. I'd do this with
> report ERROR. Otherwise, the patch looks right and worth committing.

Normally a developer is not supposed to pass NULLs there so I figured
having extra if's in the release builds doesn't worth it. Personally I
wouldn't mind using ereport() but my intuition tells me that the
committers are not going to approve this :)

Let's see what the rest of the community thinks.

-- 
Best regards,
Aleksander Alekseev

  4. Re: [PATCH] Check snapshot argument of index_beginscan and family

    Alexander Korotkov <aekorotkov@gmail.com> — 2022-12-02T15:18:48Z

    Hi!

On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev
<aleksander@timescale.com> wrote:
> Thanks for the feedback!
>
> > I think it's a nice catch and worth fixing. The one thing I don't
> > agree with is using asserts for handling the error that can appear
> > because most probably the server is built with assertions off and in
> > this case, there still will be a crash in this case. I'd do this with
> > report ERROR. Otherwise, the patch looks right and worth committing.
>
> Normally a developer is not supposed to pass NULLs there so I figured
> having extra if's in the release builds doesn't worth it. Personally I
> wouldn't mind using ereport() but my intuition tells me that the
> committers are not going to approve this :)
>
> Let's see what the rest of the community thinks.

I think this is harmless assertion patch.  I'm going to push this if
no objections.

------
Regards,
Alexander Korotkov

  5. Re: [PATCH] Check snapshot argument of index_beginscan and family

    Alexander Korotkov <aekorotkov@gmail.com> — 2022-12-06T00:31:04Z

    On Fri, Dec 2, 2022 at 6:18 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev
> <aleksander@timescale.com> wrote:
> > Thanks for the feedback!
> >
> > > I think it's a nice catch and worth fixing. The one thing I don't
> > > agree with is using asserts for handling the error that can appear
> > > because most probably the server is built with assertions off and in
> > > this case, there still will be a crash in this case. I'd do this with
> > > report ERROR. Otherwise, the patch looks right and worth committing.
> >
> > Normally a developer is not supposed to pass NULLs there so I figured
> > having extra if's in the release builds doesn't worth it. Personally I
> > wouldn't mind using ereport() but my intuition tells me that the
> > committers are not going to approve this :)
> >
> > Let's see what the rest of the community thinks.
>
> I think this is harmless assertion patch.  I'm going to push this if
> no objections.

Pushed!

------
Regards,
Alexander Korotkov

  6. Re: [PATCH] Check snapshot argument of index_beginscan and family

    Pavel Borisov <pashkin.elfe@gmail.com> — 2022-12-06T13:44:50Z

    On Tue, 6 Dec 2022 at 04:31, Alexander Korotkov <aekorotkov@gmail.com> wrote:
>
> On Fri, Dec 2, 2022 at 6:18 PM Alexander Korotkov <aekorotkov@gmail.com> wrote:
> > On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev
> > <aleksander@timescale.com> wrote:
> > > Thanks for the feedback!
> > >
> > > > I think it's a nice catch and worth fixing. The one thing I don't
> > > > agree with is using asserts for handling the error that can appear
> > > > because most probably the server is built with assertions off and in
> > > > this case, there still will be a crash in this case. I'd do this with
> > > > report ERROR. Otherwise, the patch looks right and worth committing.
> > >
> > > Normally a developer is not supposed to pass NULLs there so I figured
> > > having extra if's in the release builds doesn't worth it. Personally I
> > > wouldn't mind using ereport() but my intuition tells me that the
> > > committers are not going to approve this :)
> > >
> > > Let's see what the rest of the community thinks.
> >
> > I think this is harmless assertion patch.  I'm going to push this if
> > no objections.
>
> Pushed!

Great, thanks!
Pavel Borisov.