On Tue, 23 Aug 2022 at 13:17, Justin Pryzby <pry...@telsasoft.com> wrote:
> Attached is a squished version.
I see there's some renaming ones snuck in there. e.g:
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;
This one does not seem to be in the category I mentioned:
@@ -3036,8 +3036,6 @@ XLogFileInitInternal(XLogSegNo logsegno,
TimeLineID logtli,
pgstat_report_wait_start(WAIT_EVENT_WAL_INIT_SYNC);
if (pg_fsync(fd) != 0)
{
- int save_errno = errno;
-
More renaming:
+++ b/src/backend/catalog/heap.c
@@ -1818,19 +1818,19 @@ heap_drop_with_catalog(Oid relid)
*/
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
- Relation rel;
- HeapTuple tuple;
+ Relation pg_foreign_table;
+ HeapTuple foreigntuple;
More renaming:
+++ b/src/backend/commands/publicationcmds.c
@@ -106,7 +106,7 @@ parse_publication_options(ParseState *pstate,
{
char *publish;
List *publish_list;
- ListCell *lc;
+ ListCell *lc2;
and again:
+++ b/src/backend/commands/tablecmds.c
@@ -10223,7 +10223,7 @@ CloneFkReferencing(List **wqueue, Relation
parentRel, Relation partRel)
Oid constrOid;
ObjectAddress address,
referenced;
- ListCell *cell;
+ ListCell *lc;
I've not checked the context one this, but this does not appear to
meet the category of moving to an inner scope:
+++ b/src/backend/executor/execPartition.c
@@ -768,7 +768,6 @@ ExecInitPartitionInfo(ModifyTableState *mtstate,
EState *estate,
{
List *onconflset;
List *onconflcols;
- bool found_whole_row;
Looks like you're just using the one from the wider scope. That's not
the category we're after for now.
You've also got some renaming going on in ExecInitAgg()
- phasedata->gset_lengths[i] = perhash->numCols = aggnode->numCols;
+ phasedata->gset_lengths[setno] = perhash->numCols = aggnode->numCols;
I wondered about this one too:
- i = -1;
- while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
- aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ {
+ int i = -1;
+ while ((i = bms_next_member(all_grouped_cols, i)) >= 0)
+ aggstate->all_grouped_cols = lcons_int(i, aggstate->all_grouped_cols);
+ }
I had in mind that maybe we should switch those to be something more like:
for (int i = -1; (i = bms_next_member(all_grouped_cols, i)) >= 0;)
But I had 2nd thoughts as the "while" version has become the standard method.
(Really that code should be using bms_prev_member() and lappend_int()
so we don't have to memmove() the entire list each lcons_int() call.
(not for this patch though))
More renaming being done here:
- int i; /* Index into *ident_user */
+ int j; /* Index into *ident_user */
... in fact, there's lots of renaming, so I'll just stop looking.
Can you just send a patch that only changes the cases where you can
remove a variable declaration from an outer scope into a single inner
scope, or multiple inner scope when the variable can be declared
inside a for() loop? The mcv_get_match_bitmap() change is an example
of this. There's still a net reduction in lines of code, so I think
the mcv_get_match_bitmap(), and any like it are ok for this next step.
A counter example is ExecInitPartitionInfo() where the way to do this
would be to move the found_whole_row declaration into multiple inner
scopes. That's a net increase in code lines, for which I think
requires more careful thought if we want that or not.
David
