[PATCH] Check snapshot argument of index_beginscan and family
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]https://github.com/afiskon/postgresql-extensions/tree/main/005-table-access 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
Attachments:
v1-0001-Check-snapshot-argument-of-index_beginscan-and-fa.patchapplication/octet-stream; name=v1-0001-Check-snapshot-argument-of-index_beginscan-and-fa.patchDownload
From ae48e7bb715cb4a303f995b6cfbf6d315ce9097b Mon Sep 17 00:00:00 2001
From: Aleksander Alekseev <aleksander@timescale.com>
Date: Mon, 28 Nov 2022 12:26:50 +0300
Subject: [PATCH v1] Check snapshot argument of index_beginscan and family
Passing a NULL snapshot (InvalidSnapshot) is going to work but only as long
as the index can't find any matching rows. This can be confusing for the
extension authors so add an explicit check for this argument. The check is
implemented with Assert() in order to avoid an overhead in release builds.
Aleksander Alekseev, reported by Sven Klemm
---
src/backend/access/index/indexam.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/src/backend/access/index/indexam.c b/src/backend/access/index/indexam.c
index fe80b8b0ba..dc303995e5 100644
--- a/src/backend/access/index/indexam.c
+++ b/src/backend/access/index/indexam.c
@@ -209,6 +209,8 @@ index_beginscan(Relation heapRelation,
{
IndexScanDesc scan;
+ Assert(snapshot != InvalidSnapshot);
+
scan = index_beginscan_internal(indexRelation, nkeys, norderbys, snapshot, NULL, false);
/*
@@ -237,6 +239,8 @@ index_beginscan_bitmap(Relation indexRelation,
{
IndexScanDesc scan;
+ Assert(snapshot != InvalidSnapshot);
+
scan = index_beginscan_internal(indexRelation, nkeys, 0, snapshot, NULL, false);
/*
@@ -403,6 +407,8 @@ index_parallelscan_estimate(Relation indexRelation, Snapshot snapshot)
{
Size nbytes;
+ Assert(snapshot != InvalidSnapshot);
+
RELATION_CHECKS;
nbytes = offsetof(ParallelIndexScanDescData, ps_snapshot_data);
@@ -437,6 +443,8 @@ index_parallelscan_initialize(Relation heapRelation, Relation indexRelation,
{
Size offset;
+ Assert(snapshot != InvalidSnapshot);
+
RELATION_CHECKS;
offset = add_size(offsetof(ParallelIndexScanDescData, ps_snapshot_data),
--
2.38.1
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.
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
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
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
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.