pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix bug in Tid scan.
10+ messages / 3 participants
[nested] [flat]
Commits from this thread:
2020-02-08 4faea7fdf1 Revert recent change of ScanOption values and renumber SO_TYPE_TIDSCAN.
* pgsql: Fix bug in Tid scan.
@ 2020-02-07 13:08 Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Fujii Masao @ 2020-02-07 13:08 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix bug in Tid scan.
Commit 147e3722f7 changed Tid scan so that it calls table_beginscan()
and uses the scan option for seq scan. This change caused two issues.
(1) The change caused Tid scan to take a predicate lock on the entire
relation in serializable transaction even when relation-level
lock is not necessary. This could lead to an unexpected
serialization error.
(2) The change caused Tid scan to increment the number of seq_scan
in pg_stat_*_tables views even though it's not seq scan. This
could confuse the users.
This commit adds the scan option for Tid scan and makes Tid scan
use it, to avoid those issues.
Back-patch to v12, where the bug was introduced.
Author: Tatsuhito Kasahara
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/CAP0=ZVKy+gTbFmB6X_UW0pP3WaeJ-fkUWHoD-pExS=at3CY76g@mail.gmail.com
Branch
------
REL_12_STABLE
Details
-------
https://git.postgresql.org/pg/commitdiff/598b466e80d9f46aa1472d4f1adb1df90be8c260
Modified Files
--------------
src/backend/executor/nodeTidscan.c | 5 ++---
src/backend/utils/adt/tid.c | 4 ++--
src/include/access/tableam.h | 24 +++++++++++++++++++-----
src/test/regress/expected/tidscan.out | 16 ++++++++++++++++
src/test/regress/sql/tidscan.sql | 7 +++++++
5 files changed, 46 insertions(+), 10 deletions(-)
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
@ 2020-02-07 19:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2020-02-07 19:25 UTC (permalink / raw)
To: Fujii Masao <fujii@postgresql.org>; +Cc: pgsql-committers@lists.postgresql.org
Fujii Masao <fujii@postgresql.org> writes:
> Fix bug in Tid scan.
AFAICS, this patch has caused an ABI break in v12. Do we really
believe that no extension references the values of the ScanOptions
enum?
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-02-08 00:35 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Fujii Masao @ 2020-02-08 00:35 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
On Sat, Feb 8, 2020 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <fujii@postgresql.org> writes:
> > Fix bug in Tid scan.
>
> AFAICS, this patch has caused an ABI break in v12. Do we really
> believe that no extension references the values of the ScanOptions
> enum?
Yes, you are right. Some extensions may depend on it.
It's better not to add new ScanOption not to break ABI.
So I'm thinking to apply the attached patch only to v12.
Which fixes the issue without adding new ScanOption.
Regards,
--
Fujii Masao
Attachments:
dont_break_abi.patch (application/octet-stream, 1.2K) download
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index b5a175dffa..18ff703b14 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -47,19 +47,18 @@ typedef enum ScanOptions
SO_TYPE_SEQSCAN = 1 << 0,
SO_TYPE_BITMAPSCAN = 1 << 1,
SO_TYPE_SAMPLESCAN = 1 << 2,
- SO_TYPE_TIDSCAN = 1 << 3,
- SO_TYPE_ANALYZE = 1 << 4,
+ SO_TYPE_ANALYZE = 1 << 3,
/* several of SO_ALLOW_* may be specified */
/* allow or disallow use of access strategy */
- SO_ALLOW_STRAT = 1 << 5,
+ SO_ALLOW_STRAT = 1 << 4,
/* report location to syncscan logic? */
- SO_ALLOW_SYNC = 1 << 6,
+ SO_ALLOW_SYNC = 1 << 5,
/* verify visibility page-at-a-time? */
- SO_ALLOW_PAGEMODE = 1 << 7,
+ SO_ALLOW_PAGEMODE = 1 << 6,
/* unregister snapshot at scan end? */
- SO_TEMP_SNAPSHOT = 1 << 8
+ SO_TEMP_SNAPSHOT = 1 << 7
} ScanOptions;
/*
@@ -820,9 +819,7 @@ table_beginscan_sampling(Relation rel, Snapshot snapshot,
static inline TableScanDesc
table_beginscan_tid(Relation rel, Snapshot snapshot)
{
- uint32 flags = SO_TYPE_TIDSCAN;
-
- return rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, flags);
+ return rel->rd_tableam->scan_begin(rel, snapshot, 0, NULL, NULL, 0);
}
/*
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
@ 2020-02-08 01:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2020-02-08 01:04 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
Fujii Masao <masao.fujii@gmail.com> writes:
> On Sat, Feb 8, 2020 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> AFAICS, this patch has caused an ABI break in v12. Do we really
>> believe that no extension references the values of the ScanOptions
>> enum?
> Yes, you are right. Some extensions may depend on it.
> It's better not to add new ScanOption not to break ABI.
I think it's okay to add a new value of ScanOption; what you can't
do is change the codes assigned to the existing values. So I'd
just revert those code changes and give SO_TYPE_TIDSCAN a value
that's out-of-order.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-02-08 02:01 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 02:08 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Fujii Masao @ 2020-02-08 02:01 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
On Sat, Feb 8, 2020 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <masao.fujii@gmail.com> writes:
> > On Sat, Feb 8, 2020 at 4:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> AFAICS, this patch has caused an ABI break in v12. Do we really
> >> believe that no extension references the values of the ScanOptions
> >> enum?
>
> > Yes, you are right. Some extensions may depend on it.
> > It's better not to add new ScanOption not to break ABI.
>
> I think it's okay to add a new value of ScanOption; what you can't
> do is change the codes assigned to the existing values. So I'd
> just revert those code changes and give SO_TYPE_TIDSCAN a value
> that's out-of-order.
So you are thinking to apply something like the attached to
both master and v12? That sounds better to me.
Regards,
--
Fujii Masao
Attachments:
dont_break_abi_v2.patch (application/octet-stream, 896B) download
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 5a663975b9..3c950cf849 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -47,19 +47,19 @@ typedef enum ScanOptions
SO_TYPE_SEQSCAN = 1 << 0,
SO_TYPE_BITMAPSCAN = 1 << 1,
SO_TYPE_SAMPLESCAN = 1 << 2,
- SO_TYPE_TIDSCAN = 1 << 3,
- SO_TYPE_ANALYZE = 1 << 4,
+ SO_TYPE_ANALYZE = 1 << 3,
+ SO_TYPE_TIDSCAN = 1 << 8,
/* several of SO_ALLOW_* may be specified */
/* allow or disallow use of access strategy */
- SO_ALLOW_STRAT = 1 << 5,
+ SO_ALLOW_STRAT = 1 << 4,
/* report location to syncscan logic? */
- SO_ALLOW_SYNC = 1 << 6,
+ SO_ALLOW_SYNC = 1 << 5,
/* verify visibility page-at-a-time? */
- SO_ALLOW_PAGEMODE = 1 << 7,
+ SO_ALLOW_PAGEMODE = 1 << 6,
/* unregister snapshot at scan end? */
- SO_TEMP_SNAPSHOT = 1 << 8
+ SO_TEMP_SNAPSHOT = 1 << 7
} ScanOptions;
/*
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
@ 2020-02-08 02:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:48 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2020-02-08 02:08 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
Fujii Masao <masao.fujii@gmail.com> writes:
> On Sat, Feb 8, 2020 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> I think it's okay to add a new value of ScanOption; what you can't
>> do is change the codes assigned to the existing values. So I'd
>> just revert those code changes and give SO_TYPE_TIDSCAN a value
>> that's out-of-order.
> So you are thinking to apply something like the attached to
> both master and v12? That sounds better to me.
No, you can leave HEAD alone --- renumbering the enum values in
master is fine, since we force extensions to recompile against
new major versions. We just need to hold the values steady in
released branches.
Personally I'd keep SO_TYPE_TIDSCAN physically adjacent to the other
SO_TYPE_xxxSCAN entries in the list, but of course that's just cosmetic.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 02:08 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-02-08 02:48 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 03:05 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 10+ messages in thread
From: Fujii Masao @ 2020-02-08 02:48 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
On Sat, Feb 8, 2020 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <masao.fujii@gmail.com> writes:
> > On Sat, Feb 8, 2020 at 10:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> I think it's okay to add a new value of ScanOption; what you can't
> >> do is change the codes assigned to the existing values. So I'd
> >> just revert those code changes and give SO_TYPE_TIDSCAN a value
> >> that's out-of-order.
>
> > So you are thinking to apply something like the attached to
> > both master and v12? That sounds better to me.
>
> No, you can leave HEAD alone --- renumbering the enum values in
> master is fine, since we force extensions to recompile against
> new major versions. We just need to hold the values steady in
> released branches.
Yeah, you're right.
> Personally I'd keep SO_TYPE_TIDSCAN physically adjacent to the other
> SO_TYPE_xxxSCAN entries in the list, but of course that's just cosmetic.
+1. So I will apply the latest patch (adding SO_TYPE_TIDSCAN just after
SO_TYPE_ANALYZE) only to v12.
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 02:08 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:48 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
@ 2020-02-08 03:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 03:30 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
0 siblings, 1 reply; 10+ messages in thread
From: Tom Lane @ 2020-02-08 03:05 UTC (permalink / raw)
To: Fujii Masao <masao.fujii@gmail.com>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
Fujii Masao <masao.fujii@gmail.com> writes:
> On Sat, Feb 8, 2020 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>> Personally I'd keep SO_TYPE_TIDSCAN physically adjacent to the other
>> SO_TYPE_xxxSCAN entries in the list, but of course that's just cosmetic.
> +1. So I will apply the latest patch (adding SO_TYPE_TIDSCAN just after
> SO_TYPE_ANALYZE) only to v12.
Works for me.
regards, tom lane
^ permalink raw reply [nested|flat] 10+ messages in thread
* Re: pgsql: Fix bug in Tid scan.
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 02:08 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:48 ` Re: pgsql: Fix bug in Tid scan. Fujii Masao <masao.fujii@gmail.com>
2020-02-08 03:05 ` Re: pgsql: Fix bug in Tid scan. Tom Lane <tgl@sss.pgh.pa.us>
@ 2020-02-08 03:30 ` Fujii Masao <masao.fujii@gmail.com>
0 siblings, 0 replies; 10+ messages in thread
From: Fujii Masao @ 2020-02-08 03:30 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: Fujii Masao <fujii@postgresql.org>; pgsql-committers@lists.postgresql.org
On Sat, Feb 8, 2020 at 12:05 PM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Fujii Masao <masao.fujii@gmail.com> writes:
> > On Sat, Feb 8, 2020 at 11:08 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> >> Personally I'd keep SO_TYPE_TIDSCAN physically adjacent to the other
> >> SO_TYPE_xxxSCAN entries in the list, but of course that's just cosmetic.
>
> > +1. So I will apply the latest patch (adding SO_TYPE_TIDSCAN just after
> > SO_TYPE_ANALYZE) only to v12.
>
> Works for me.
Pushed! Thanks a lot!!
Regards,
--
Fujii Masao
^ permalink raw reply [nested|flat] 10+ messages in thread
* pgsql: Fix bug in Tid scan.
@ 2020-02-07 13:08 Fujii Masao <fujii@postgresql.org>
0 siblings, 0 replies; 10+ messages in thread
From: Fujii Masao @ 2020-02-07 13:08 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix bug in Tid scan.
Commit 147e3722f7 changed Tid scan so that it calls table_beginscan()
and uses the scan option for seq scan. This change caused two issues.
(1) The change caused Tid scan to take a predicate lock on the entire
relation in serializable transaction even when relation-level
lock is not necessary. This could lead to an unexpected
serialization error.
(2) The change caused Tid scan to increment the number of seq_scan
in pg_stat_*_tables views even though it's not seq scan. This
could confuse the users.
This commit adds the scan option for Tid scan and makes Tid scan
use it, to avoid those issues.
Back-patch to v12, where the bug was introduced.
Author: Tatsuhito Kasahara
Reviewed-by: Kyotaro Horiguchi, Masahiko Sawada, Fujii Masao
Discussion: https://postgr.es/m/CAP0=ZVKy+gTbFmB6X_UW0pP3WaeJ-fkUWHoD-pExS=at3CY76g@mail.gmail.com
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/cb5b28613d553b1c750622e91cbc96c83f052a63
Modified Files
--------------
src/backend/executor/nodeTidscan.c | 5 ++---
src/backend/utils/adt/tid.c | 4 ++--
src/include/access/tableam.h | 24 +++++++++++++++++++-----
src/test/regress/expected/tidscan.out | 16 ++++++++++++++++
src/test/regress/sql/tidscan.sql | 7 +++++++
5 files changed, 46 insertions(+), 10 deletions(-)
^ permalink raw reply [nested|flat] 10+ messages in thread
end of thread, other threads:[~2020-02-08 03:30 UTC | newest]
Thread overview: 10+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
2020-02-07 19:25 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 00:35 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 01:04 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:01 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 02:08 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 02:48 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-08 03:05 ` Tom Lane <tgl@sss.pgh.pa.us>
2020-02-08 03:30 ` Fujii Masao <masao.fujii@gmail.com>
2020-02-07 13:08 pgsql: Fix bug in Tid scan. Fujii Masao <fujii@postgresql.org>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox