Re: [PATCH] Check snapshot argument of index_beginscan and family
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.
Re: [PATCH] Check snapshot argument of index_beginscan and family
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
Re: [PATCH] Check snapshot argument of index_beginscan and family
Hi! On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev 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
Re: [PATCH] Check snapshot argument of index_beginscan and family
On Fri, Dec 2, 2022 at 6:18 PM Alexander Korotkov wrote: > On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev > 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
Re: [PATCH] Check snapshot argument of index_beginscan and family
On Tue, 6 Dec 2022 at 04:31, Alexander Korotkov wrote: > > On Fri, Dec 2, 2022 at 6:18 PM Alexander Korotkov > wrote: > > On Mon, Nov 28, 2022 at 1:30 PM Aleksander Alekseev > > 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.