public inbox for [email protected]
help / color / mirror / Atom feedRe: Add 64-bit XIDs into PostgreSQL 15
29+ messages / 7 participants
[nested] [flat]
* Re: Add 64-bit XIDs into PostgreSQL 15
@ 2022-12-09 12:49 Aleksander Alekseev <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Aleksander Alekseev @ 2022-12-09 12:49 UTC (permalink / raw)
To: [email protected] <[email protected]>; +Cc: adherent postgres <[email protected]>; Chris Travers <[email protected]>; Chris Travers <[email protected]>; Bruce Momjian <[email protected]>
Hi adherent,
> Robertmhaas said that the project Zheap is dead（https://twitter.com/andy_pavlo/status/1590703943176589312）, which means that we cannot use Zheap to deal with the issue of xid wraparound and dead tuples in tables. The dead tuple issue is not a big deal because I can still use pg_repack to handle, although pg_repack will cause wal log to increase dramatically and may take one or two days to handle a large table. During this time the database can be accessed by external users, but the xid wraparound will cause PostgreSQL to be down, which is a disaster for DBAs. Maybe you are not a DBA, or your are from a small country, Database system tps is very low, so xid32 is enough for your database system , Oracle's scn was also 32bits, however, Oracle realized the issue and changed scn to 64 bits. The transaction id in mysql is 48 bits. MySQL didn't fix the transaction id wraparound problem because they think that 48 bits is enough for the transaction id. This project has been running for almost 1 year and now it is coming to an end. I strongly disagree with your idea of stopping this patch, and I suspect you are a saboteur. I strongly disagree with your viewpoint, as it is not a fundamental way to solve the xid wraparound problem. The PostgreSQL community urgently needs developers who solve problems like this, not bury one' head in the sand
This is not uncommon for people on the mailing list to have
disagreements. This is part of the process, we all are looking for
consensus. It's true that different people have different use cases in
mind and different backgrounds as well. It doesn't mean these use
cases are wrong and/or the experience is irrelevant and/or the
received feedback should be just discarded.
Although I also expressed my disagreement with Chris before, let's not
assume any bad intent and especially sabotage as you put it. (Unless
you have a strong proof of this of course which I doubt you have.) We
want all kinds of feedback to be welcomed here. I'm sure our goal here
is mutual, to make PostgreSQL even better than it is now. The only
problem is that the definition of "better" varies sometimes.
I see you believe that 64-bit XIDs are going to be useful. That's
great! Tell us more about your case and how the patch is going to help
with it. Also, maybe you could test your load with the applied
patchset and tell us whether it makes things better or worse?
Personally I would love hearing this from you.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
@ 2022-12-09 13:54 adherent postgres <[email protected]>
parent: Aleksander Alekseev <[email protected]>
0 siblings, 2 replies; 29+ messages in thread
From: adherent postgres @ 2022-12-09 13:54 UTC (permalink / raw)
To: Aleksander Alekseev <[email protected]>; [email protected] <[email protected]>; +Cc: Chris Travers <[email protected]>; Chris Travers <[email protected]>; Bruce Momjian <[email protected]>
Hi Aleksander Alekseev
I think the xids 32bit transformation project has been dragged on for too long. Huawei's openGauss referenced this patch to implement xids 64bit, and Postgrespro also implemented xids 64bit, which is enough to prove that their worries are redundant.I think postgresql has no reason not to implement xid 64 bit. What about your opinion?
Best whish
________________________________
发件人: Aleksander Alekseev <[email protected]>
发送时间: 2022年12月9日 20:49
收件人: [email protected] <[email protected]>
抄送: adherent postgres <[email protected]>; Chris Travers <[email protected]>; Chris Travers <[email protected]>; Bruce Momjian <[email protected]>
主题: Re: Add 64-bit XIDs into PostgreSQL 15
Hi adherent,
> Robertmhaas said that the project Zheap is dead（https://twitter.com/andy_pavlo/status/1590703943176589312）, which means that we cannot use Zheap to deal with the issue of xid wraparound and dead tuples in tables. The dead tuple issue is not a big deal because I can still use pg_repack to handle, although pg_repack will cause wal log to increase dramatically and may take one or two days to handle a large table. During this time the database can be accessed by external users, but the xid wraparound will cause PostgreSQL to be down, which is a disaster for DBAs. Maybe you are not a DBA, or your are from a small country, Database system tps is very low, so xid32 is enough for your database system , Oracle's scn was also 32bits, however, Oracle realized the issue and changed scn to 64 bits. The transaction id in mysql is 48 bits. MySQL didn't fix the transaction id wraparound problem because they think that 48 bits is enough for the transaction id. This project has been running for almost 1 year and now it is coming to an end. I strongly disagree with your idea of stopping this patch, and I suspect you are a saboteur. I strongly disagree with your viewpoint, as it is not a fundamental way to solve the xid wraparound problem. The PostgreSQL community urgently needs developers who solve problems like this, not bury one' head in the sand
This is not uncommon for people on the mailing list to have
disagreements. This is part of the process, we all are looking for
consensus. It's true that different people have different use cases in
mind and different backgrounds as well. It doesn't mean these use
cases are wrong and/or the experience is irrelevant and/or the
received feedback should be just discarded.
Although I also expressed my disagreement with Chris before, let's not
assume any bad intent and especially sabotage as you put it. (Unless
you have a strong proof of this of course which I doubt you have.) We
want all kinds of feedback to be welcomed here. I'm sure our goal here
is mutual, to make PostgreSQL even better than it is now. The only
problem is that the definition of "better" varies sometimes.
I see you believe that 64-bit XIDs are going to be useful. That's
great! Tell us more about your case and how the patch is going to help
with it. Also, maybe you could test your load with the applied
patchset and tell us whether it makes things better or worse?
Personally I would love hearing this from you.
--
Best regards,
Aleksander Alekseev
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
@ 2022-12-09 14:13 Pavel Borisov <[email protected]>
parent: adherent postgres <[email protected]>
1 sibling, 0 replies; 29+ messages in thread
From: Pavel Borisov @ 2022-12-09 14:13 UTC (permalink / raw)
To: adherent postgres <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; [email protected] <[email protected]>; Chris Travers <[email protected]>; Chris Travers <[email protected]>; Bruce Momjian <[email protected]>
Hi, Adherent!
On Fri, 9 Dec 2022 at 17:54, adherent postgres
<[email protected]> wrote:
>
> Hi Aleksander Alekseev
> I think the xids 32bit transformation project has been dragged on for too long. Huawei's openGauss referenced this patch to implement xids 64bit, and Postgrespro also implemented xids 64bit, which is enough to prove that their worries are redundant.I think postgresql has no reason not to implement xid 64 bit. What about your opinion?
I agree it's high time to commit 64xids into PostgreSQL.
If you can do your review of the whole proposed patchset or only the
part that is likely to be committed earlier [1] it would help a lot!
I'd recommend beginning with the last version of the patch in thread
[1]. First, it is easier. Also, this review is going to be useful
sooner and will help a committer on January commitfest a lot.
[1]: https://www.postgresql.org/message-id/CAFiTN-uudj2PY8GsUzFtLYFpBoq_rKegW3On_8ZHdxB1mVv3-A%40mail.gma...
Regards,
Pavel Borisov,
Supabase
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Add 64-bit XIDs into PostgreSQL 15
@ 2022-12-09 14:29 Maxim Orlov <[email protected]>
parent: adherent postgres <[email protected]>
1 sibling, 0 replies; 29+ messages in thread
From: Maxim Orlov @ 2022-12-09 14:29 UTC (permalink / raw)
To: adherent postgres <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; [email protected] <[email protected]>; Chris Travers <[email protected]>; Chris Travers <[email protected]>; Bruce Momjian <[email protected]>
On Fri, 9 Dec 2022 at 16:54, adherent postgres <
[email protected]> wrote:
> Hi Aleksander Alekseev
> I think the xids 32bit transformation project has been dragged on for too
> long. Huawei's openGauss referenced this patch to implement xids 64bit, and
> Postgrespro also implemented xids 64bit, which is enough to prove that
> their worries are redundant.I think postgresql has no reason not to
> implement xid 64 bit. What about your opinion?
>
Yeah, I totally agree, the time has come. With a high transaction load,
Postgres become more and more difficult to maintain.
The problem is in the overall complexity of the patch set. We need more
reviewers.
Since committing such a big patch is not viable once at a time, from the
start of the work we did split it into several logical parts.
The evolution concept is more preferable in this case. As far as I can see,
there is overall consensus to commit SLRU related
changes first.
--
Best regards,
Maxim Orlov.
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-02-20 03:01 Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Kyotaro Horiguchi @ 2023-02-20 03:01 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
At Fri, 17 Feb 2023 13:53:36 +0300, Nazir Bilal Yavuz <[email protected]> wrote in
> Thanks for the review. I updated the patch.
WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
- PendingWalStats.wal_records = diff.wal_records;
- PendingWalStats.wal_fpi = diff.wal_fpi;
- PendingWalStats.wal_bytes = diff.wal_bytes;
...
+ WALSTAT_ACC(wal_records, diff);
+ WALSTAT_ACC(wal_fpi, diff);
+ WALSTAT_ACC(wal_bytes, diff);
+ WALSTAT_ACC(wal_buffers_full, PendingWalStats);
The lifetime of the variable "diff" seems to be longer now. Wouldn't
it be clearer if we renamed it to something more meaningful, like
wal_usage_diff, WalUsageDiff or PendingWalUsage? Along those same
lines, it occurs to me that the new struct should be named
PgStat_PendingWalStats, instead of ..Usage. That change makes the name
of the type and the variable consistent.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-02-21 13:11 Nazir Bilal Yavuz <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nazir Bilal Yavuz @ 2023-02-21 13:11 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; pgsql-hackers
Hi,
Thanks for the review.
On Mon, 20 Feb 2023 at 06:01, Kyotaro Horiguchi <[email protected]> wrote:
>
> At Fri, 17 Feb 2023 13:53:36 +0300, Nazir Bilal Yavuz <[email protected]> wrote in
> > Thanks for the review. I updated the patch.
>
>
> WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
> - PendingWalStats.wal_records = diff.wal_records;
> - PendingWalStats.wal_fpi = diff.wal_fpi;
> - PendingWalStats.wal_bytes = diff.wal_bytes;
> ...
> + WALSTAT_ACC(wal_records, diff);
> + WALSTAT_ACC(wal_fpi, diff);
> + WALSTAT_ACC(wal_bytes, diff);
> + WALSTAT_ACC(wal_buffers_full, PendingWalStats);
>
>
> The lifetime of the variable "diff" seems to be longer now. Wouldn't
> it be clearer if we renamed it to something more meaningful, like
> wal_usage_diff, WalUsageDiff or PendingWalUsage? Along those same
> lines, it occurs to me that the new struct should be named
> PgStat_PendingWalStats, instead of ..Usage. That change makes the name
> of the type and the variable consistent.
I agree. The patch is updated.
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[application/octet-stream] v3-0001-Refactor-instr_time-calculations.patch (5.6K, ../../CAN55FZ2p=bW5hTPT_8LYcrKNvtcdhYNxrDgTjqJF7YNX9JSaEw@mail.gmail.com/2-v3-0001-Refactor-instr_time-calculations.patch)
download | inline diff:
From f8a7e590b9bfb240bf807d32543192c9486d1d21 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Thu, 2 Feb 2023 15:06:48 +0300
Subject: [PATCH v3] Refactor instr_time calculations
---
src/backend/access/transam/xlog.c | 6 ++---
src/backend/storage/file/buffile.c | 6 ++---
src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
src/include/pgstat.h | 18 +++++++++++++-
4 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9f0f6db8d..3c35dc1ca2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2206,8 +2206,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_write_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_write_time, duration, start);
}
PendingWalStats.wal_write++;
@@ -8201,8 +8200,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_sync_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start);
}
PendingWalStats.wal_sync++;
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 0a51624df3..e55f86b675 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -469,8 +469,7 @@ BufFileLoadBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_read_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_read_time, io_time, io_start);
}
/* we choose not to advance curOffset here */
@@ -544,8 +543,7 @@ BufFileDumpBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_write_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_write_time, io_time, io_start);
}
file->curOffset += bytestowrite;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e8598b2f4e..0979d1c269 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -21,7 +21,7 @@
#include "executor/instrument.h"
-PgStat_WalStats PendingWalStats = {0};
+PgStat_PendingWalStats PendingWalStats = {0};
/*
* WAL usage counters saved from pgWALUsage at the previous call to
@@ -70,7 +70,7 @@ bool
pgstat_flush_wal(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
- WalUsage diff = {0};
+ WalUsage wal_usage_diff = {0};
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(pgStatLocal.shmem != NULL &&
@@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
* Calculate how much WAL usage counters were increased by subtracting the
* previous counters from the current ones.
*/
- WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
- PendingWalStats.wal_records = diff.wal_records;
- PendingWalStats.wal_fpi = diff.wal_fpi;
- PendingWalStats.wal_bytes = diff.wal_bytes;
+ WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
if (!nowait)
LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
return true;
-#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
- WALSTAT_ACC(wal_records);
- WALSTAT_ACC(wal_fpi);
- WALSTAT_ACC(wal_bytes);
- WALSTAT_ACC(wal_buffers_full);
- WALSTAT_ACC(wal_write);
- WALSTAT_ACC(wal_sync);
- WALSTAT_ACC(wal_write_time);
- WALSTAT_ACC(wal_sync_time);
+#define WALSTAT_ACC(fld, var_to_add) \
+ (stats_shmem->stats.fld += var_to_add.fld)
+#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
+ (stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+ WALSTAT_ACC(wal_records, wal_usage_diff);
+ WALSTAT_ACC(wal_fpi, wal_usage_diff);
+ WALSTAT_ACC(wal_bytes, wal_usage_diff);
+ WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+ WALSTAT_ACC(wal_write, PendingWalStats);
+ WALSTAT_ACC(wal_sync, PendingWalStats);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
+#undef WALLSTAT_ACC_INSTR_TIME_TYPE
#undef WALSTAT_ACC
LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f..4be6fdeaf5 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -445,6 +445,22 @@ typedef struct PgStat_WalStats
TimestampTz stat_reset_timestamp;
} PgStat_WalStats;
+/*
+ * Created for accumulating wal_write_time and wal_sync_time as a instr_time
+ * but instr_time can't be used as a type where it ends up on-disk
+ * because its units may change. PgStat_WalStats type is used for
+ * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
+ * accumulating intervals as a instr_time.
+ */
+typedef struct PgStat_PendingWalStats
+{
+ PgStat_Counter wal_buffers_full;
+ PgStat_Counter wal_write;
+ PgStat_Counter wal_sync;
+ instr_time wal_write_time;
+ instr_time wal_sync_time;
+} PgStat_PendingWalStats;
+
/*
* Functions in pgstat.c
@@ -758,7 +774,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
*/
/* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats;
#endif /* PGSTAT_H */
--
2.25.1
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-02-22 02:50 Kyotaro Horiguchi <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Kyotaro Horiguchi @ 2023-02-22 02:50 UTC (permalink / raw)
To: [email protected]; +Cc: [email protected]; pgsql-hackers
At Tue, 21 Feb 2023 16:11:19 +0300, Nazir Bilal Yavuz <[email protected]> wrote in
> I agree. The patch is updated.
Thanks, that part looks good to me. I'd like to provide some
additional comments.
PgStat_PendingStats should be included in typedefs.list.
+ * Created for accumulating wal_write_time and wal_sync_time as a instr_time
+ * but instr_time can't be used as a type where it ends up on-disk
+ * because its units may change. PgStat_WalStats type is used for
+ * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
+ * accumulating intervals as a instr_time.
+ */
+typedef struct PgStat_PendingWalStats
IMHO, this comment looks somewhat off. Maybe we could try something
like the following instead?
> This struct stores wal-related durations as instr_time, which makes it
> easier to accumulate them without requiring type conversions. Then,
> during stats flush, they will be moved into shared stats with type
> conversions.
The aim of this patch is to keep using instr_time for accumulation.
So it seems like we could do the same refactoring for
pgStatBlockReadTime, pgStatBlockWriteTime, pgStatActiveTime and
pgStatTransactionIdleTime. What do you think - should we include this
additional refactoring in the same patch or make a separate one for
it?
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-02-22 10:13 Nazir Bilal Yavuz <[email protected]>
parent: Kyotaro Horiguchi <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nazir Bilal Yavuz @ 2023-02-22 10:13 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; pgsql-hackers
Hi,
Thanks for the review.
On Wed, 22 Feb 2023 at 05:50, Kyotaro Horiguchi <[email protected]> wrote:
>
> PgStat_PendingStats should be included in typedefs.list.
Done.
>
> + * Created for accumulating wal_write_time and wal_sync_time as a instr_time
> + * but instr_time can't be used as a type where it ends up on-disk
> + * because its units may change. PgStat_WalStats type is used for
> + * in-memory/on-disk data. So, PgStat_PendingWalStats is created for
> + * accumulating intervals as a instr_time.
> + */
> +typedef struct PgStat_PendingWalStats
>
> IMHO, this comment looks somewhat off. Maybe we could try something
> like the following instead?
>
> > This struct stores wal-related durations as instr_time, which makes it
> > easier to accumulate them without requiring type conversions. Then,
> > during stats flush, they will be moved into shared stats with type
> > conversions.
Done. And I think we should write why we didn't change
PgStat_WalStats's variable types and instead created a new struct.
Maybe we can explain it in the commit description?
>
> The aim of this patch is to keep using instr_time for accumulation.
> So it seems like we could do the same refactoring for
> pgStatBlockReadTime, pgStatBlockWriteTime, pgStatActiveTime and
> pgStatTransactionIdleTime. What do you think - should we include this
> additional refactoring in the same patch or make a separate one for
> it?
I tried a bit but it seems the required changes for additional
refactoring aren't small. So, I think we can create a separate patch
for these changes.
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[application/octet-stream] v4-0001-Refactor-instr_time-calculations.patch (6.0K, ../../CAN55FZ0-svc-om2F2jGMTyTgVit9kKJ7wzy4nfhWJRZjkB4fKw@mail.gmail.com/2-v4-0001-Refactor-instr_time-calculations.patch)
download | inline diff:
From 9c5d7a96aed730e9d77496890742f8188d107343 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Thu, 2 Feb 2023 15:06:48 +0300
Subject: [PATCH v4] Refactor instr_time calculations
---
src/backend/access/transam/xlog.c | 6 ++---
src/backend/storage/file/buffile.c | 6 ++---
src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
src/include/pgstat.h | 17 +++++++++++++-
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f9f0f6db8d..3c35dc1ca2 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2206,8 +2206,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_write_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_write_time, duration, start);
}
PendingWalStats.wal_write++;
@@ -8201,8 +8200,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_sync_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start);
}
PendingWalStats.wal_sync++;
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 0a51624df3..e55f86b675 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -469,8 +469,7 @@ BufFileLoadBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_read_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_read_time, io_time, io_start);
}
/* we choose not to advance curOffset here */
@@ -544,8 +543,7 @@ BufFileDumpBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_write_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_write_time, io_time, io_start);
}
file->curOffset += bytestowrite;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e8598b2f4e..0979d1c269 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -21,7 +21,7 @@
#include "executor/instrument.h"
-PgStat_WalStats PendingWalStats = {0};
+PgStat_PendingWalStats PendingWalStats = {0};
/*
* WAL usage counters saved from pgWALUsage at the previous call to
@@ -70,7 +70,7 @@ bool
pgstat_flush_wal(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
- WalUsage diff = {0};
+ WalUsage wal_usage_diff = {0};
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(pgStatLocal.shmem != NULL &&
@@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
* Calculate how much WAL usage counters were increased by subtracting the
* previous counters from the current ones.
*/
- WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
- PendingWalStats.wal_records = diff.wal_records;
- PendingWalStats.wal_fpi = diff.wal_fpi;
- PendingWalStats.wal_bytes = diff.wal_bytes;
+ WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
if (!nowait)
LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
return true;
-#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
- WALSTAT_ACC(wal_records);
- WALSTAT_ACC(wal_fpi);
- WALSTAT_ACC(wal_bytes);
- WALSTAT_ACC(wal_buffers_full);
- WALSTAT_ACC(wal_write);
- WALSTAT_ACC(wal_sync);
- WALSTAT_ACC(wal_write_time);
- WALSTAT_ACC(wal_sync_time);
+#define WALSTAT_ACC(fld, var_to_add) \
+ (stats_shmem->stats.fld += var_to_add.fld)
+#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
+ (stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+ WALSTAT_ACC(wal_records, wal_usage_diff);
+ WALSTAT_ACC(wal_fpi, wal_usage_diff);
+ WALSTAT_ACC(wal_bytes, wal_usage_diff);
+ WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+ WALSTAT_ACC(wal_write, PendingWalStats);
+ WALSTAT_ACC(wal_sync, PendingWalStats);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
+#undef WALLSTAT_ACC_INSTR_TIME_TYPE
#undef WALSTAT_ACC
LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index db9675884f..82dd7d6416 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -445,6 +445,21 @@ typedef struct PgStat_WalStats
TimestampTz stat_reset_timestamp;
} PgStat_WalStats;
+/*
+ * This struct stores wal-related durations as instr_time, which makes it
+ * easier to accumulate them without requiring type conversions. Then,
+ * during stats flush, they will be moved into shared stats with type
+ * conversions.
+ */
+typedef struct PgStat_PendingWalStats
+{
+ PgStat_Counter wal_buffers_full;
+ PgStat_Counter wal_write;
+ PgStat_Counter wal_sync;
+ instr_time wal_write_time;
+ instr_time wal_sync_time;
+} PgStat_PendingWalStats;
+
/*
* Functions in pgstat.c
@@ -758,7 +773,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
*/
/* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats;
#endif /* PGSTAT_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 22ea42c16b..5d28aa1ca9 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2045,6 +2045,7 @@ PgStat_Kind
PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
+PgStat_PendingWalStats
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.25.1
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-03-09 13:02 Nazir Bilal Yavuz <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Nazir Bilal Yavuz @ 2023-03-09 13:02 UTC (permalink / raw)
To: Kyotaro Horiguchi <[email protected]>; +Cc: [email protected]; pgsql-hackers
Hi,
There was a warning while applying the patch, v5 is attached.
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[text/x-diff] v5-0001-Refactor-instr_time-calculations.patch (6.5K, ../../CAN55FZ0FXnCuOAt5LsLcmNd1zP5Tg1DfD5vB7=hNUMqCpGHO9w@mail.gmail.com/2-v5-0001-Refactor-instr_time-calculations.patch)
download | inline diff:
From dcd49e48a0784a95b8731df1c6ee7c3a612a8529 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Thu, 9 Mar 2023 15:35:38 +0300
Subject: [PATCH v5] Refactor instr_time calculations
In instr_time.h it is stated that:
* When summing multiple measurements, it's recommended to leave the
* running sum in instr_time form (ie, use INSTR_TIME_ADD or
* INSTR_TIME_ACCUM_DIFF) and convert to a result format only at the end.
So, this patch aims to refactor 'PendingWalStats' to use 'instr_time' instead
of 'PgStat_Counter' while accumulating 'wal_write_time' and 'wal_sync_time'.
Also, some calculations are refactored to use 'INSTR_TIME_ACCUM_DIFF' instead
of using 'INSTR_TIME_SUBTRACT' and 'INSTR_TIME_ADD'.
---
src/backend/access/transam/xlog.c | 6 ++---
src/backend/storage/file/buffile.c | 6 ++---
src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
src/include/pgstat.h | 17 +++++++++++++-
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 543d4d897ae..46821ad6056 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2206,8 +2206,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_write_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_write_time, duration, start);
}
PendingWalStats.wal_write++;
@@ -8204,8 +8203,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_sync_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start);
}
PendingWalStats.wal_sync++;
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 0a51624df3b..e55f86b675e 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -469,8 +469,7 @@ BufFileLoadBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_read_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_read_time, io_time, io_start);
}
/* we choose not to advance curOffset here */
@@ -544,8 +543,7 @@ BufFileDumpBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_write_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_write_time, io_time, io_start);
}
file->curOffset += bytestowrite;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e8598b2f4e0..58daae3fbd6 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -21,7 +21,7 @@
#include "executor/instrument.h"
-PgStat_WalStats PendingWalStats = {0};
+PgStat_PendingWalStats PendingWalStats = {0};
/*
* WAL usage counters saved from pgWALUsage at the previous call to
@@ -70,7 +70,7 @@ bool
pgstat_flush_wal(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
- WalUsage diff = {0};
+ WalUsage wal_usage_diff = {0};
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(pgStatLocal.shmem != NULL &&
@@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
* Calculate how much WAL usage counters were increased by subtracting the
* previous counters from the current ones.
*/
- WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
- PendingWalStats.wal_records = diff.wal_records;
- PendingWalStats.wal_fpi = diff.wal_fpi;
- PendingWalStats.wal_bytes = diff.wal_bytes;
+ WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
if (!nowait)
LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
return true;
-#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
- WALSTAT_ACC(wal_records);
- WALSTAT_ACC(wal_fpi);
- WALSTAT_ACC(wal_bytes);
- WALSTAT_ACC(wal_buffers_full);
- WALSTAT_ACC(wal_write);
- WALSTAT_ACC(wal_sync);
- WALSTAT_ACC(wal_write_time);
- WALSTAT_ACC(wal_sync_time);
+#define WALSTAT_ACC(fld, var_to_add) \
+ (stats_shmem->stats.fld += var_to_add.fld)
+#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
+ (stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+ WALSTAT_ACC(wal_records, wal_usage_diff);
+ WALSTAT_ACC(wal_fpi, wal_usage_diff);
+ WALSTAT_ACC(wal_bytes, wal_usage_diff);
+ WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+ WALSTAT_ACC(wal_write, PendingWalStats);
+ WALSTAT_ACC(wal_sync, PendingWalStats);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
+ WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
+#undef WALLSTAT_ACC_INSTR_TIME_TYPE
#undef WALSTAT_ACC
LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index f43fac09ede..5bbc55bb341 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -442,6 +442,21 @@ typedef struct PgStat_WalStats
TimestampTz stat_reset_timestamp;
} PgStat_WalStats;
+/*
+ * This struct stores wal-related durations as instr_time, which makes it
+ * easier to accumulate them without requiring type conversions. Then,
+ * during stats flush, they will be moved into shared stats with type
+ * conversions.
+ */
+typedef struct PgStat_PendingWalStats
+{
+ PgStat_Counter wal_buffers_full;
+ PgStat_Counter wal_write;
+ PgStat_Counter wal_sync;
+ instr_time wal_write_time;
+ instr_time wal_sync_time;
+} PgStat_PendingWalStats;
+
/*
* Functions in pgstat.c
@@ -755,7 +770,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
*/
/* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats;
#endif /* PGSTAT_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 86a9303bf56..68eb52886fa 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2049,6 +2049,7 @@ PgStat_Kind
PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
+PgStat_PendingWalStats
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.39.2
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-03-16 23:02 Melanie Plageman <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 1 reply; 29+ messages in thread
From: Melanie Plageman @ 2023-03-16 23:02 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
On Thu, Mar 09, 2023 at 04:02:44PM +0300, Nazir Bilal Yavuz wrote:
> From dcd49e48a0784a95b8731df1c6ee7c3a612a8529 Mon Sep 17 00:00:00 2001
> From: Nazir Bilal Yavuz <[email protected]>
> Date: Thu, 9 Mar 2023 15:35:38 +0300
> Subject: [PATCH v5] Refactor instr_time calculations
>
> Also, some calculations are refactored to use 'INSTR_TIME_ACCUM_DIFF' instead
> of using 'INSTR_TIME_SUBTRACT' and 'INSTR_TIME_ADD'.
> ---
> src/backend/access/transam/xlog.c | 6 ++---
> src/backend/storage/file/buffile.c | 6 ++---
> src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
> src/include/pgstat.h | 17 +++++++++++++-
> src/tools/pgindent/typedefs.list | 1 +
> 5 files changed, 37 insertions(+), 24 deletions(-)
>
> diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
> index e8598b2f4e0..58daae3fbd6 100644
> --- a/src/backend/utils/activity/pgstat_wal.c
> +++ b/src/backend/utils/activity/pgstat_wal.c
> @@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
> * Calculate how much WAL usage counters were increased by subtracting the
> * previous counters from the current ones.
> */
> - WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
> - PendingWalStats.wal_records = diff.wal_records;
> - PendingWalStats.wal_fpi = diff.wal_fpi;
> - PendingWalStats.wal_bytes = diff.wal_bytes;
> + WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
>
> if (!nowait)
> LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
> else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
> return true;
>
> -#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
> - WALSTAT_ACC(wal_records);
> - WALSTAT_ACC(wal_fpi);
> - WALSTAT_ACC(wal_bytes);
> - WALSTAT_ACC(wal_buffers_full);
> - WALSTAT_ACC(wal_write);
> - WALSTAT_ACC(wal_sync);
> - WALSTAT_ACC(wal_write_time);
> - WALSTAT_ACC(wal_sync_time);
> +#define WALSTAT_ACC(fld, var_to_add) \
> + (stats_shmem->stats.fld += var_to_add.fld)
> +#define WALLSTAT_ACC_INSTR_TIME_TYPE(fld) \
> + (stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
> + WALSTAT_ACC(wal_records, wal_usage_diff);
> + WALSTAT_ACC(wal_fpi, wal_usage_diff);
> + WALSTAT_ACC(wal_bytes, wal_usage_diff);
> + WALSTAT_ACC(wal_buffers_full, PendingWalStats);
> + WALSTAT_ACC(wal_write, PendingWalStats);
> + WALSTAT_ACC(wal_sync, PendingWalStats);
> + WALLSTAT_ACC_INSTR_TIME_TYPE(wal_write_time);
I think you want one less L here?
WALLSTAT_ACC_INSTR_TIME_TYPE -> WALSTAT_ACC_INSTR_TIME_TYPE
Also, I don't quite understand why TYPE is at the end of the name. I
think it would still be clear without it.
I might find it clearer if the WALSTAT_ACC_INSTR_TIME_TYPE macro was
defined before using it for those fields instead of defining it right
after defining WALSTAT_ACC.
> + WALLSTAT_ACC_INSTR_TIME_TYPE(wal_sync_time);
> +#undef WALLSTAT_ACC_INSTR_TIME_TYPE
> #undef WALSTAT_ACC
>
> LWLockRelease(&stats_shmem->lock);
> diff --git a/src/include/pgstat.h b/src/include/pgstat.h
> index f43fac09ede..5bbc55bb341 100644
> --- a/src/include/pgstat.h
> +++ b/src/include/pgstat.h
> @@ -442,6 +442,21 @@ typedef struct PgStat_WalStats
> TimestampTz stat_reset_timestamp;
> } PgStat_WalStats;
>
> +/*
> + * This struct stores wal-related durations as instr_time, which makes it
> + * easier to accumulate them without requiring type conversions. Then,
> + * during stats flush, they will be moved into shared stats with type
> + * conversions.
> + */
> +typedef struct PgStat_PendingWalStats
> +{
> + PgStat_Counter wal_buffers_full;
> + PgStat_Counter wal_write;
> + PgStat_Counter wal_sync;
> + instr_time wal_write_time;
> + instr_time wal_sync_time;
> +} PgStat_PendingWalStats;
> +
So, I am not a fan of having this second struct (PgStat_PendingWalStats)
which only has a subset of the members of PgStat_WalStats. It is pretty
confusing.
It is okay to have two structs -- one that is basically "in-memory" and
one that is a format that can be on disk, but these two structs with
different members are confusing and don't convey why we have the two
structs.
I would either put WalUsage into PgStat_PendingWalStats (so that it has
all the same members as PgStat_WalStats), or figure out a way to
maintain WalUsage separately from PgStat_WalStats or something else.
Worst case, add more comments to the struct definitions to explain why
they have the members they have and how WalUsage relates to them.
- Melanie
^ permalink raw reply [nested|flat] 29+ messages in thread
* Re: Refactor calculations to use instr_time
@ 2023-03-23 14:38 Nazir Bilal Yavuz <[email protected]>
parent: Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Nazir Bilal Yavuz @ 2023-03-23 14:38 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; Kyotaro Horiguchi <[email protected]>; [email protected]; pgsql-hackers
Hi,
Thanks for the review.
On Fri, 17 Mar 2023 at 02:02, Melanie Plageman
<[email protected]> wrote:
> I think you want one less L here?
> WALLSTAT_ACC_INSTR_TIME_TYPE -> WALSTAT_ACC_INSTR_TIME_TYPE
Done.
> Also, I don't quite understand why TYPE is at the end of the name. I
> think it would still be clear without it.
Done.
> I might find it clearer if the WALSTAT_ACC_INSTR_TIME_TYPE macro was
> defined before using it for those fields instead of defining it right
> after defining WALSTAT_ACC.
Since it is undefined together with WALSTAT_ACC, defining them
together makes sense to me.
> > + * This struct stores wal-related durations as instr_time, which makes it
> > + * easier to accumulate them without requiring type conversions. Then,
> > + * during stats flush, they will be moved into shared stats with type
> > + * conversions.
> > + */
> > +typedef struct PgStat_PendingWalStats
> > +{
> > + PgStat_Counter wal_buffers_full;
> > + PgStat_Counter wal_write;
> > + PgStat_Counter wal_sync;
> > + instr_time wal_write_time;
> > + instr_time wal_sync_time;
> > +} PgStat_PendingWalStats;
> > +
>
> So, I am not a fan of having this second struct (PgStat_PendingWalStats)
> which only has a subset of the members of PgStat_WalStats. It is pretty
> confusing.
>
> It is okay to have two structs -- one that is basically "in-memory" and
> one that is a format that can be on disk, but these two structs with
> different members are confusing and don't convey why we have the two
> structs.
>
> I would either put WalUsage into PgStat_PendingWalStats (so that it has
> all the same members as PgStat_WalStats), or figure out a way to
> maintain WalUsage separately from PgStat_WalStats or something else.
> Worst case, add more comments to the struct definitions to explain why
> they have the members they have and how WalUsage relates to them.
Yes, but like Andres said this could be better done as a separate patch.
v6 is attached.
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[application/x-patch] v6-0001-Refactor-instr_time-calculations.patch (6.5K, ../../CAN55FZ2kHG4jzaVcErbQosLcW=RCsKZ_5TSW39oEBibcLzQkig@mail.gmail.com/2-v6-0001-Refactor-instr_time-calculations.patch)
download | inline diff:
From 02f1b5426ad9394a673c28a6df53fc56ff6ffa89 Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Wed, 22 Mar 2023 15:25:44 +0300
Subject: [PATCH v6] Refactor instr_time calculations
In instr_time.h it is stated that:
* When summing multiple measurements, it's recommended to leave the
* running sum in instr_time form (ie, use INSTR_TIME_ADD or
* INSTR_TIME_ACCUM_DIFF) and convert to a result format only at the end.
So, this patch aims to refactor 'PendingWalStats' to use 'instr_time' instead
of 'PgStat_Counter' while accumulating 'wal_write_time' and 'wal_sync_time'.
Also, some calculations are refactored to use 'INSTR_TIME_ACCUM_DIFF' instead
of using 'INSTR_TIME_SUBTRACT' and 'INSTR_TIME_ADD'.
---
src/backend/access/transam/xlog.c | 6 ++---
src/backend/storage/file/buffile.c | 6 ++---
src/backend/utils/activity/pgstat_wal.c | 31 +++++++++++++------------
src/include/pgstat.h | 17 +++++++++++++-
src/tools/pgindent/typedefs.list | 1 +
5 files changed, 37 insertions(+), 24 deletions(-)
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 543d4d897ae..46821ad6056 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -2206,8 +2206,7 @@ XLogWrite(XLogwrtRqst WriteRqst, TimeLineID tli, bool flexible)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_write_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_write_time, duration, start);
}
PendingWalStats.wal_write++;
@@ -8204,8 +8203,7 @@ issue_xlog_fsync(int fd, XLogSegNo segno, TimeLineID tli)
instr_time duration;
INSTR_TIME_SET_CURRENT(duration);
- INSTR_TIME_SUBTRACT(duration, start);
- PendingWalStats.wal_sync_time += INSTR_TIME_GET_MICROSEC(duration);
+ INSTR_TIME_ACCUM_DIFF(PendingWalStats.wal_sync_time, duration, start);
}
PendingWalStats.wal_sync++;
diff --git a/src/backend/storage/file/buffile.c b/src/backend/storage/file/buffile.c
index 0a51624df3b..e55f86b675e 100644
--- a/src/backend/storage/file/buffile.c
+++ b/src/backend/storage/file/buffile.c
@@ -469,8 +469,7 @@ BufFileLoadBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_read_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_read_time, io_time, io_start);
}
/* we choose not to advance curOffset here */
@@ -544,8 +543,7 @@ BufFileDumpBuffer(BufFile *file)
if (track_io_timing)
{
INSTR_TIME_SET_CURRENT(io_time);
- INSTR_TIME_SUBTRACT(io_time, io_start);
- INSTR_TIME_ADD(pgBufferUsage.temp_blk_write_time, io_time);
+ INSTR_TIME_ACCUM_DIFF(pgBufferUsage.temp_blk_write_time, io_time, io_start);
}
file->curOffset += bytestowrite;
diff --git a/src/backend/utils/activity/pgstat_wal.c b/src/backend/utils/activity/pgstat_wal.c
index e8598b2f4e0..94f1a3b06e3 100644
--- a/src/backend/utils/activity/pgstat_wal.c
+++ b/src/backend/utils/activity/pgstat_wal.c
@@ -21,7 +21,7 @@
#include "executor/instrument.h"
-PgStat_WalStats PendingWalStats = {0};
+PgStat_PendingWalStats PendingWalStats = {0};
/*
* WAL usage counters saved from pgWALUsage at the previous call to
@@ -70,7 +70,7 @@ bool
pgstat_flush_wal(bool nowait)
{
PgStatShared_Wal *stats_shmem = &pgStatLocal.shmem->wal;
- WalUsage diff = {0};
+ WalUsage wal_usage_diff = {0};
Assert(IsUnderPostmaster || !IsPostmasterEnvironment);
Assert(pgStatLocal.shmem != NULL &&
@@ -88,25 +88,26 @@ pgstat_flush_wal(bool nowait)
* Calculate how much WAL usage counters were increased by subtracting the
* previous counters from the current ones.
*/
- WalUsageAccumDiff(&diff, &pgWalUsage, &prevWalUsage);
- PendingWalStats.wal_records = diff.wal_records;
- PendingWalStats.wal_fpi = diff.wal_fpi;
- PendingWalStats.wal_bytes = diff.wal_bytes;
+ WalUsageAccumDiff(&wal_usage_diff, &pgWalUsage, &prevWalUsage);
if (!nowait)
LWLockAcquire(&stats_shmem->lock, LW_EXCLUSIVE);
else if (!LWLockConditionalAcquire(&stats_shmem->lock, LW_EXCLUSIVE))
return true;
-#define WALSTAT_ACC(fld) stats_shmem->stats.fld += PendingWalStats.fld
- WALSTAT_ACC(wal_records);
- WALSTAT_ACC(wal_fpi);
- WALSTAT_ACC(wal_bytes);
- WALSTAT_ACC(wal_buffers_full);
- WALSTAT_ACC(wal_write);
- WALSTAT_ACC(wal_sync);
- WALSTAT_ACC(wal_write_time);
- WALSTAT_ACC(wal_sync_time);
+#define WALSTAT_ACC(fld, var_to_add) \
+ (stats_shmem->stats.fld += var_to_add.fld)
+#define WALSTAT_ACC_INSTR_TIME(fld) \
+ (stats_shmem->stats.fld += INSTR_TIME_GET_MICROSEC(PendingWalStats.fld))
+ WALSTAT_ACC(wal_records, wal_usage_diff);
+ WALSTAT_ACC(wal_fpi, wal_usage_diff);
+ WALSTAT_ACC(wal_bytes, wal_usage_diff);
+ WALSTAT_ACC(wal_buffers_full, PendingWalStats);
+ WALSTAT_ACC(wal_write, PendingWalStats);
+ WALSTAT_ACC(wal_sync, PendingWalStats);
+ WALSTAT_ACC_INSTR_TIME(wal_write_time);
+ WALSTAT_ACC_INSTR_TIME(wal_sync_time);
+#undef WALSTAT_ACC_INSTR_TIME
#undef WALSTAT_ACC
LWLockRelease(&stats_shmem->lock);
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index 1e418b682b5..e8e17423c47 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -433,6 +433,21 @@ typedef struct PgStat_WalStats
TimestampTz stat_reset_timestamp;
} PgStat_WalStats;
+/*
+ * This struct stores wal-related durations as instr_time, which makes it
+ * easier to accumulate them without requiring type conversions. Then,
+ * during stats flush, they will be moved into shared stats with type
+ * conversions.
+ */
+typedef struct PgStat_PendingWalStats
+{
+ PgStat_Counter wal_buffers_full;
+ PgStat_Counter wal_write;
+ PgStat_Counter wal_sync;
+ instr_time wal_write_time;
+ instr_time wal_sync_time;
+} PgStat_PendingWalStats;
+
/*
* Functions in pgstat.c
@@ -746,7 +761,7 @@ extern PGDLLIMPORT SessionEndType pgStatSessionEndCause;
*/
/* updated directly by backends and background processes */
-extern PGDLLIMPORT PgStat_WalStats PendingWalStats;
+extern PGDLLIMPORT PgStat_PendingWalStats PendingWalStats;
#endif /* PGSTAT_H */
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 097f42e1b34..879f748a7dc 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -2048,6 +2048,7 @@ PgStat_Kind
PgStat_KindInfo
PgStat_LocalState
PgStat_PendingDroppedStatsItem
+PgStat_PendingWalStats
PgStat_SLRUStats
PgStat_ShmemControl
PgStat_Snapshot
--
2.40.0
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v10-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 38b4596a775..08e346ae7b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0008-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index a6b98fa12a1..b2397fe2054 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v10-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v11 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5ba8cb3657..ce47a158ae 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2172,7 +2172,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2300,6 +2301,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index ae758857bd..5c2a7b7422 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -815,6 +815,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -830,7 +833,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -2023,14 +2027,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2041,7 +2047,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v12 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 4c5fe8f090d..973734e9ffa 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2199,7 +2199,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2327,6 +2328,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595ec..c95e3412da8 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index e8d04b4dec6..19e99c81092 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -811,6 +811,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -826,7 +829,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -2013,14 +2017,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2031,7 +2037,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--6jpz2j246qmht4bt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v12-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 38b4596a775..08e346ae7b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0008-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index a6b98fa12a1..b2397fe2054 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v10-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v11 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 5ba8cb3657..ce47a158ae 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2172,7 +2172,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2300,6 +2301,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index ae758857bd..5c2a7b7422 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -815,6 +815,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -830,7 +833,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -2023,14 +2027,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2041,7 +2047,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 38b4596a775..08e346ae7b3 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0008-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index c9b9b4c00f1..10c1c3b616b 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2112,7 +2112,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2240,6 +2241,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index a6b98fa12a1..b2397fe2054 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index c43a8b3dea5..f1d0d4b78e3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -796,6 +796,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -811,7 +814,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1951,14 +1955,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -1969,7 +1975,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
* [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact
@ 2024-02-27 01:34 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 29+ messages in thread
From: Melanie Plageman @ 2024-02-27 01:34 UTC (permalink / raw)
Future commits will remove the TBMIterateResult from BitmapHeapNext() --
pushing it into the table AM-specific code. So, the table AM must inform
BitmapHeapNext() whether or not the current block is lossy or exact for
the purposes of the counters used in EXPLAIN.
---
src/backend/access/heap/heapam_handler.c | 5 ++++-
src/backend/executor/nodeBitmapHeapscan.c | 10 +++++-----
src/include/access/tableam.h | 14 ++++++++++----
3 files changed, 19 insertions(+), 10 deletions(-)
diff --git a/src/backend/access/heap/heapam_handler.c b/src/backend/access/heap/heapam_handler.c
index 7fdccaf613..849cac3947 100644
--- a/src/backend/access/heap/heapam_handler.c
+++ b/src/backend/access/heap/heapam_handler.c
@@ -2114,7 +2114,8 @@ heapam_estimate_rel_size(Relation rel, int32 *attr_widths,
static bool
heapam_scan_bitmap_next_block(TableScanDesc scan,
- TBMIterateResult *tbmres)
+ TBMIterateResult *tbmres,
+ bool *lossy)
{
HeapScanDesc hscan = (HeapScanDesc) scan;
BlockNumber block = tbmres->blockno;
@@ -2242,6 +2243,8 @@ heapam_scan_bitmap_next_block(TableScanDesc scan,
Assert(ntup <= MaxHeapTuplesPerPage);
hscan->rs_ntuples = ntup;
+ *lossy = tbmres->ntuples < 0;
+
return ntup > 0;
}
diff --git a/src/backend/executor/nodeBitmapHeapscan.c b/src/backend/executor/nodeBitmapHeapscan.c
index 404de0595e..c95e3412da 100644
--- a/src/backend/executor/nodeBitmapHeapscan.c
+++ b/src/backend/executor/nodeBitmapHeapscan.c
@@ -211,7 +211,7 @@ BitmapHeapNext(BitmapHeapScanState *node)
for (;;)
{
- bool valid;
+ bool valid, lossy;
CHECK_FOR_INTERRUPTS();
@@ -232,12 +232,12 @@ BitmapHeapNext(BitmapHeapScanState *node)
BitmapAdjustPrefetchIterator(node, tbmres->blockno);
- valid = table_scan_bitmap_next_block(scan, tbmres);
+ valid = table_scan_bitmap_next_block(scan, tbmres, &lossy);
- if (tbmres->ntuples >= 0)
- node->exact_pages++;
- else
+ if (lossy)
node->lossy_pages++;
+ else
+ node->exact_pages++;
if (!valid)
{
diff --git a/src/include/access/tableam.h b/src/include/access/tableam.h
index 1bc5f7c057..b9ba4f9fb3 100644
--- a/src/include/access/tableam.h
+++ b/src/include/access/tableam.h
@@ -804,6 +804,9 @@ typedef struct TableAmRoutine
* on the page have to be returned, otherwise the tuples at offsets in
* `tbmres->offsets` need to be returned.
*
+ * lossy indicates whether or not the block's representation in the bitmap
+ * is lossy or exact.
+ *
* XXX: Currently this may only be implemented if the AM uses md.c as its
* storage manager, and uses ItemPointer->ip_blkid in a manner that maps
* blockids directly to the underlying storage. nodeBitmapHeapscan.c
@@ -819,7 +822,8 @@ typedef struct TableAmRoutine
* scan_bitmap_next_tuple need to exist, or neither.
*/
bool (*scan_bitmap_next_block) (TableScanDesc scan,
- struct TBMIterateResult *tbmres);
+ struct TBMIterateResult *tbmres,
+ bool *lossy);
/*
* Fetch the next tuple of a bitmap table scan into `slot` and return true
@@ -1988,14 +1992,16 @@ table_relation_estimate_size(Relation rel, int32 *attr_widths,
* Prepare to fetch / check / return tuples from `tbmres->blockno` as part of
* a bitmap table scan. `scan` needs to have been started via
* table_beginscan_bm(). Returns false if there are no tuples to be found on
- * the page, true otherwise.
+ * the page, true otherwise. lossy is set to true if bitmap is lossy for the
+ * selected block and false otherwise.
*
* Note, this is an optionally implemented function, therefore should only be
* used after verifying the presence (at plan time or such).
*/
static inline bool
table_scan_bitmap_next_block(TableScanDesc scan,
- struct TBMIterateResult *tbmres)
+ struct TBMIterateResult *tbmres,
+ bool *lossy)
{
/*
* We don't expect direct calls to table_scan_bitmap_next_block with valid
@@ -2006,7 +2012,7 @@ table_scan_bitmap_next_block(TableScanDesc scan,
elog(ERROR, "unexpected table_scan_bitmap_next_block call during logical decoding");
return scan->rs_rd->rd_tableam->scan_bitmap_next_block(scan,
- tbmres);
+ tbmres, lossy);
}
/*
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0007-Reduce-scope-of-BitmapHeapScan-tbmiterator-local-.patch"
^ permalink raw reply [nested|flat] 29+ messages in thread
end of thread, other threads:[~2024-02-27 01:34 UTC | newest]
Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2022-12-09 12:49 Re: Add 64-bit XIDs into PostgreSQL 15 Aleksander Alekseev <[email protected]>
2022-12-09 13:54 ` adherent postgres <[email protected]>
2022-12-09 14:13 ` Pavel Borisov <[email protected]>
2022-12-09 14:29 ` Maxim Orlov <[email protected]>
2023-02-20 03:01 Re: Refactor calculations to use instr_time Kyotaro Horiguchi <[email protected]>
2023-02-21 13:11 ` Re: Refactor calculations to use instr_time Nazir Bilal Yavuz <[email protected]>
2023-02-22 02:50 ` Re: Refactor calculations to use instr_time Kyotaro Horiguchi <[email protected]>
2023-02-22 10:13 ` Re: Refactor calculations to use instr_time Nazir Bilal Yavuz <[email protected]>
2023-03-09 13:02 ` Re: Refactor calculations to use instr_time Nazir Bilal Yavuz <[email protected]>
2023-03-16 23:02 ` Re: Refactor calculations to use instr_time Melanie Plageman <[email protected]>
2023-03-23 14:38 ` Re: Refactor calculations to use instr_time Nazir Bilal Yavuz <[email protected]>
2024-02-27 01:34 [PATCH v11 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v12 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v11 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v10 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v7 06/13] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v6 07/14] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v8 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
2024-02-27 01:34 [PATCH v9 06/17] table_scan_bitmap_next_block() returns lossy or exact Melanie Plageman <[email protected]>
This inbox is served by agora; see mirroring instructions
for how to clone and mirror all data and code used for this inbox