Thread: [HACKERS] 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
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Attachment
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
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