Unused variable scanned_tuples in LVRelStats
Hi,
scanned_tuples variable in LVRelStats is introduced by commit b4b6923e
but it seems to me that it's actually not used. We store num_tuples
into vacrelstats->scanned_tuples after scanned all blocks, and the
comment mentioned that saving it in order to use later but we actually
use num_tuples instead of vacrelstats->scanned_tuples from there. I
think the since the name of scanned_tuples implies more clearer
purpose than num_tuples it's better to use it instead of num_tuples,
or we can remove scanned_tuples from LVRelStats.
/* save stats for use later */
vacrelstats->scanned_tuples = num_tuples;
vacrelstats->tuples_deleted = tups_vacuumed;
vacrelstats->new_dead_tuples = nkeep;
Attached small path fixes this.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
fix_vacuumlazy.patchapplication/octet-stream; name=fix_vacuumlazy.patchDownload
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index 881c735..0edd49d 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -1268,7 +1268,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false,
nblocks,
vacrelstats->tupcount_pages,
- num_tuples);
+ vacrelstats->scanned_tuples);
/*
* Release any remaining pin on visibility map page.
@@ -1357,7 +1357,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
ereport(elevel,
(errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages",
RelationGetRelationName(onerel),
- tups_vacuumed, num_tuples,
+ tups_vacuumed, vacrelstats->scanned_tuples,
vacrelstats->scanned_pages, nblocks),
errdetail_internal("%s", buf.data)));
pfree(buf.data);
On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
scanned_tuples variable in LVRelStats is introduced by commit b4b6923e
but it seems to me that it's actually not used. We store num_tuples
into vacrelstats->scanned_tuples after scanned all blocks, and the
comment mentioned that saving it in order to use later but we actually
use num_tuples instead of vacrelstats->scanned_tuples from there. I
think the since the name of scanned_tuples implies more clearer
purpose than num_tuples it's better to use it instead of num_tuples,
or we can remove scanned_tuples from LVRelStats.
I think we should only store stuff in LVRelStats if it needs to be
passed to some other function. Data that's only used in
lazy_scan_heap() can just be kept in local variables. We could rename
the local variable, though, since I agree with you that scanned_tuples
is clearer.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Wed, Aug 2, 2017 at 11:40 PM, Robert Haas <robertmhaas@gmail.com> wrote:
On Tue, Jul 4, 2017 at 10:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
scanned_tuples variable in LVRelStats is introduced by commit b4b6923e
but it seems to me that it's actually not used. We store num_tuples
into vacrelstats->scanned_tuples after scanned all blocks, and the
comment mentioned that saving it in order to use later but we actually
use num_tuples instead of vacrelstats->scanned_tuples from there. I
think the since the name of scanned_tuples implies more clearer
purpose than num_tuples it's better to use it instead of num_tuples,
or we can remove scanned_tuples from LVRelStats.
Thank you for the comment!
I think we should only store stuff in LVRelStats if it needs to be
passed to some other function.
Agreed. From this point of view, num_tuples is only one variable of
LVRelStats that is not passed to other functions.
Data that's only used in
lazy_scan_heap() can just be kept in local variables. We could rename
the local variable, though, since I agree with you that scanned_tuples
is clearer.
So we can remove scanned_tuples from LVRelStats struct and change the
variable name num_tuples to scanned_tuples. Attached updated patch.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
Attachments:
fix_vacuumlazy_v2.patchapplication/octet-stream; name=fix_vacuumlazy_v2.patchDownload
diff --git a/src/backend/commands/vacuumlazy.c b/src/backend/commands/vacuumlazy.c
index fabb2f8..1b418d9 100644
--- a/src/backend/commands/vacuumlazy.c
+++ b/src/backend/commands/vacuumlazy.c
@@ -114,7 +114,6 @@ typedef struct LVRelStats
BlockNumber pinskipped_pages; /* # of pages we skipped due to a pin */
BlockNumber frozenskipped_pages; /* # of frozen pages we skipped */
BlockNumber tupcount_pages; /* pages whose tuples we counted */
- double scanned_tuples; /* counts only tuples on tupcount_pages */
double old_rel_tuples; /* previous value of pg_class.reltuples */
double new_rel_tuples; /* new estimated total # of tuples */
double new_dead_tuples; /* new estimated total # of dead tuples */
@@ -464,7 +463,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
char *relname;
BlockNumber empty_pages,
vacuumed_pages;
- double num_tuples,
+ double scanned_tuples, /* counts only tuples on vacrelstats->tupcount_pages */
tups_vacuumed,
nkeep,
nunused;
@@ -492,7 +491,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
relname)));
empty_pages = vacuumed_pages = 0;
- num_tuples = tups_vacuumed = nkeep = nunused = 0;
+ scanned_tuples = tups_vacuumed = nkeep = nunused = 0;
indstats = (IndexBulkDeleteResult **)
palloc0(nindexes * sizeof(IndexBulkDeleteResult *));
@@ -1077,7 +1076,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
{
bool tuple_totally_frozen;
- num_tuples += 1;
+ scanned_tuples += 1;
hastup = true;
/*
@@ -1259,7 +1258,6 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
pfree(frozen);
/* save stats for use later */
- vacrelstats->scanned_tuples = num_tuples;
vacrelstats->tuples_deleted = tups_vacuumed;
vacrelstats->new_dead_tuples = nkeep;
@@ -1267,7 +1265,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
vacrelstats->new_rel_tuples = vac_estimate_reltuples(onerel, false,
nblocks,
vacrelstats->tupcount_pages,
- num_tuples);
+ scanned_tuples);
/*
* Release any remaining pin on visibility map page.
@@ -1356,7 +1354,7 @@ lazy_scan_heap(Relation onerel, int options, LVRelStats *vacrelstats,
ereport(elevel,
(errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages",
RelationGetRelationName(onerel),
- tups_vacuumed, num_tuples,
+ tups_vacuumed, scanned_tuples,
vacrelstats->scanned_pages, nblocks),
errdetail_internal("%s", buf.data)));
pfree(buf.data);
On Thu, Aug 3, 2017 at 1:10 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
So we can remove scanned_tuples from LVRelStats struct and change the
variable name num_tuples to scanned_tuples. Attached updated patch.
On second thought, I think we should just leave this alone.
scanned_tuples is closely tied to tupcount_pages, so it's a little
confusing to pull one out and not the other. And we can't pull
tupcount_pages out of LVRelStats because lazy_cleanup_index needs it.
The current situation isn't doing any harm, so I'm not seeing much
point in changing it.
--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
On Fri, Aug 4, 2017 at 3:01 AM, Robert Haas <robertmhaas@gmail.com> wrote:
On Thu, Aug 3, 2017 at 1:10 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:
So we can remove scanned_tuples from LVRelStats struct and change the
variable name num_tuples to scanned_tuples. Attached updated patch.On second thought, I think we should just leave this alone.
scanned_tuples is closely tied to tupcount_pages, so it's a little
confusing to pull one out and not the other. And we can't pull
tupcount_pages out of LVRelStats because lazy_cleanup_index needs it.
The current situation isn't doing any harm, so I'm not seeing much
point in changing it.
Hmm, since I agree the current situation isn't doing any harm actually
I don't push hard for this but I'd like to still propose at least the
original patch that fixes an inconsistent in the code.
Regards,
--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers