De: Kyotaro Horiguchi <horikyota....@gmail.com>
Enviado: segunda-feira, 9 de dezembro de 2019 03:40
>The file-scoped variable is needed to be process-persistent in any
>way. If we inhibit them, the upper-modules need to create the
>persistent area instead, for example, by calling XLogInitGlobals() or
>such, which makes things messier. Globality doens't necessarily mean
>evil and there're reasons for -Wall doesn't warn the case. I believe
>we, and especially committers are not who should be kept away from
>knives for the reason that knives generally have a possibility to
>injure someone.
Which harms the reusability of the code anyway.
>I might be too accustomed there, but the functions that define
>overriding locals don't modify the local variables and only the
>functions that don't override the globals modifies the glboals. I see
>no significant confusion here. By the way changes like "conf_file" ->
>"conffile" seems really useless as a fix patch.
Well i was trying to fix everything.
>As Robert said, they are harmless as far as we notice. Actual bugs
>caused by variable overriding would be welcomed to fix. I don't
>believe "lead to better performance and reduction (of code?)" without
>an evidence since modern compilers I think are not so stupid. Even if
>any, performance change in such extent doesn't support the proposal to
>remove variable overrides that way.
It's clear to me now that unless "the thing" is clearly a bug, don't touch it.
I love C, so for me it's very hard to resist getting stupid things like:
foo ()
{
int i, n;
for (i-0; i < n; i ++);
{
int i;
for (i=0; i < n; i ++);
}
{
int i;
for (i=0; i < n; i ++);
}
return;
I don't know how you can do it.
Of course, there are cases and cases, let's look at the example of multixact.c
diff --git a / src / backend / access / transam / multixact.c b / src / backend
/ access / transam / multixact.c
index 7b2448e05b..6364014fb3 100644
--- a / src / backend / access / transam / multixact.c
+++ b / src / backend / access / transam / multixact.c
@@ -1589.10 +1589.10 @@ mXactCachePut (MultiXactId multi, int nmembers,
MultiXactMember * members)
qsort (entry-> members, nmembers, sizeof (MultiXactMember),
mxactMemberComparator);
dlist_push_head (& MXactCache, & entry-> node);
+ pfree (entry); // <- is it really necessary?
if (MXactCacheMembers ++> = MAX_CACHE_ENTRIES)
{
dlist_node * node;
- mXactCacheEnt * entry;
node = dlist_tail_node (& MXactCache);
dlist_delete (node);
I still can't decide if it's a bug or not.
If it is a bug the correct function here is pfree or what is the equivalent
function to free memory?
>Anyway I strongly object to the name 'pRedoRecPtr', which suggests as
>if it is a C-pointer to some variable. (And I believe we use Hungarian
>notation only if we don't have a better way...) LatestRedoRecPtr
>looks better to me.
I don't have enough information to decide if the lastest is the proper name, so
I tried to change the nomenclature as little as possible.
I'll submit a patch sample, which depending on the answer, will give me if it's
worth it or not, keep working on it.
regards,
Ranier Vileladiff --git a/src/backend/access/common/reloptions.c b/src/backend/access/common/reloptions.c
index 48377ace24..1bad56991e 100644
--- a/src/backend/access/common/reloptions.c
+++ b/src/backend/access/common/reloptions.c
@@ -1198,7 +1198,6 @@ parseRelOptions(Datum options, bool validate, relopt_kind kind,
{
char *text_str = VARDATA(optiondatums[i]);
int text_len = VARSIZE(optiondatums[i]) - VARHDRSZ;
- int j;
/* Search for a match in reloptions */
for (j = 0; j < numoptions; j++)
diff --git a/src/backend/access/gin/ginget.c b/src/backend/access/gin/ginget.c
index b18ae2b3ed..78291fe696 100644
--- a/src/backend/access/gin/ginget.c
+++ b/src/backend/access/gin/ginget.c
@@ -392,7 +392,6 @@ restartScanEntry:
{
BlockNumber rootPostingTree = GinGetPostingTree(itup);
GinBtreeStack *stack;
- Page page;
ItemPointerData minItem;
/*
diff --git a/src/backend/access/gist/gist.c b/src/backend/access/gist/gist.c
index 3800f58ce7..43204b61f7 100644
--- a/src/backend/access/gist/gist.c
+++ b/src/backend/access/gist/gist.c
@@ -366,7 +366,6 @@ gistplacetopage(Relation rel, Size freespace, GISTSTATE *giststate,
{
IndexTuple *downlinks;
int ndownlinks = 0;
- int i;
rootpg.buffer = buffer;
rootpg.page = PageGetTempPageCopySpecial(BufferGetPage(rootpg.buffer));
diff --git a/src/backend/access/gist/gistbuild.c b/src/backend/access/gist/gistbuild.c
index 739846a257..8b721f60cd 100644
--- a/src/backend/access/gist/gistbuild.c
+++ b/src/backend/access/gist/gistbuild.c
@@ -763,7 +763,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
i = 0;
foreach(lc, splitinfo)
{
- GISTPageSplitInfo *splitinfo = lfirst(lc);
+ GISTPageSplitInfo *split = lfirst(lc);
/*
* Remember the parent of each new child page in our parent map.
@@ -774,7 +774,7 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
*/
if (level > 0)
gistMemorizeParent(buildstate,
- BufferGetBlockNumber(splitinfo->buf),
+ BufferGetBlockNumber(split->buf),
BufferGetBlockNumber(parentBuffer));
/*
@@ -784,14 +784,14 @@ gistbufferinginserttuples(GISTBuildState *buildstate, Buffer buffer, int level,
* harm).
*/
if (level > 1)
- gistMemorizeAllDownlinks(buildstate, splitinfo->buf);
+ gistMemorizeAllDownlinks(buildstate, split->buf);
/*
* Since there's no concurrent access, we can release the lower
* level buffers immediately. This includes the original page.
*/
- UnlockReleaseBuffer(splitinfo->buf);
- downlinks[i++] = splitinfo->downlink;
+ UnlockReleaseBuffer(split->buf);
+ downlinks[i++] = split->downlink;
}
/* Insert them into parent. */
diff --git a/src/backend/access/gist/gistbuildbuffers.c b/src/backend/access/gist/gistbuildbuffers.c
index 38f786848d..c3990a5fff 100644
--- a/src/backend/access/gist/gistbuildbuffers.c
+++ b/src/backend/access/gist/gistbuildbuffers.c
@@ -635,8 +635,7 @@ gistRelocateBuildBuffersOnSplit(GISTBuildBuffers *gfbb, GISTSTATE *giststate,
while (gistPopItupFromNodeBuffer(gfbb, &oldBuf, &itup))
{
float best_penalty[INDEX_MAX_KEYS];
- int i,
- which;
+ int which;
IndexTuple newtup;
RelocationBufferInfo *targetBufferInfo;
diff --git a/src/backend/access/hash/hash_xlog.c b/src/backend/access/hash/hash_xlog.c
index c1d248ae5e..98735ab719 100644
--- a/src/backend/access/hash/hash_xlog.c
+++ b/src/backend/access/hash/hash_xlog.c
@@ -240,7 +240,6 @@ hash_xlog_add_ovfl_page(XLogReaderState *record)
{
Page mappage = (Page) BufferGetPage(mapbuffer);
uint32 *freep = NULL;
- char *data;
uint32 *bitmap_page_bit;
freep = HashPageGetBitmap(mappage);
diff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index 0128bb34ef..6a101f542b 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -5961,7 +5961,7 @@ FreezeMultiXactId(MultiXactId multi, uint16 t_infomask,
*/
if (ISUPDATE_from_mxstatus(members[i].status))
{
- TransactionId xid = members[i].xid;
+ xid = members[i].xid;
Assert(TransactionIdIsValid(xid));
if (TransactionIdPrecedes(xid, relfrozenxid))
diff --git a/src/backend/access/heap/vacuumlazy.c b/src/backend/access/heap/vacuumlazy.c
index a3c4a1df3b..f6aae66457 100644
--- a/src/backend/access/heap/vacuumlazy.c
+++ b/src/backend/access/heap/vacuumlazy.c
@@ -517,7 +517,7 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
BlockNumber next_unskippable_block;
bool skipping_blocks;
xl_heap_freeze_tuple *frozen;
- StringInfoData buf;
+ StringInfoData str;
const int initprog_index[] = {
PROGRESS_VACUUM_PHASE,
PROGRESS_VACUUM_TOTAL_HEAP_BLKS,
@@ -918,10 +918,10 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
if (GetRecordedFreeSpace(onerel, blkno) == 0)
{
- Size freespace;
+ Size freesize;
- freespace = BufferGetPageSize(buf) - SizeOfPageHeaderData;
- RecordPageWithFreeSpace(onerel, blkno, freespace);
+ freesize = BufferGetPageSize(buf) - SizeOfPageHeaderData;
+ RecordPageWithFreeSpace(onerel, blkno, freesize);
}
}
continue;
@@ -1481,33 +1481,33 @@ lazy_scan_heap(Relation onerel, VacuumParams *params, LVRelStats *vacrelstats,
* This is pretty messy, but we split it up so that we can skip emitting
* individual parts of the message when not applicable.
*/
- initStringInfo(&buf);
- appendStringInfo(&buf,
+ initStringInfo(&str);
+ appendStringInfo(&str,
_("%.0f dead row versions cannot be removed yet, oldest xmin: %u\n"),
nkeep, OldestXmin);
- appendStringInfo(&buf, _("There were %.0f unused item identifiers.\n"),
+ appendStringInfo(&str, _("There were %.0f unused item identifiers.\n"),
nunused);
- appendStringInfo(&buf, ngettext("Skipped %u page due to buffer pins, ",
+ appendStringInfo(&str, ngettext("Skipped %u page due to buffer pins, ",
"Skipped %u pages due to buffer pins, ",
vacrelstats->pinskipped_pages),
vacrelstats->pinskipped_pages);
- appendStringInfo(&buf, ngettext("%u frozen page.\n",
+ appendStringInfo(&str, ngettext("%u frozen page.\n",
"%u frozen pages.\n",
vacrelstats->frozenskipped_pages),
vacrelstats->frozenskipped_pages);
- appendStringInfo(&buf, ngettext("%u page is entirely empty.\n",
+ appendStringInfo(&str, ngettext("%u page is entirely empty.\n",
"%u pages are entirely empty.\n",
empty_pages),
empty_pages);
- appendStringInfo(&buf, _("%s."), pg_rusage_show(&ru0));
+ appendStringInfo(&str, _("%s."), pg_rusage_show(&ru0));
ereport(elevel,
(errmsg("\"%s\": found %.0f removable, %.0f nonremovable row versions in %u out of %u pages",
RelationGetRelationName(onerel),
tups_vacuumed, num_tuples,
vacrelstats->scanned_pages, nblocks),
- errdetail_internal("%s", buf.data)));
- pfree(buf.data);
+ errdetail_internal("%s", str.data)));
+ pfree(str.data);
}
diff --git a/src/backend/access/spgist/spgdoinsert.c b/src/backend/access/spgist/spgdoinsert.c
index c34c44cd8b..03f6aa689c 100644
--- a/src/backend/access/spgist/spgdoinsert.c
+++ b/src/backend/access/spgist/spgdoinsert.c
@@ -420,8 +420,6 @@ moveLeafs(Relation index, SpGistState *state,
i = current->offnum;
while (i != InvalidOffsetNumber)
{
- SpGistLeafTuple it;
-
Assert(i >= FirstOffsetNumber &&
i <= PageGetMaxOffsetNumber(current->page));
it = (SpGistLeafTuple) PageGetItem(current->page,
diff --git a/src/backend/access/transam/clog.c b/src/backend/access/transam/clog.c
index 7f7eb21b4f..6446748a84 100644
--- a/src/backend/access/transam/clog.c
+++ b/src/backend/access/transam/clog.c
@@ -422,6 +422,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
{
volatile PROC_HDR *procglobal = ProcGlobal;
PGPROC *proc = MyProc;
+ PGXACT *pgxact;
uint32 nextidx;
uint32 wakeidx;
@@ -516,8 +517,8 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
/* Walk the list and update the status of all XIDs. */
while (nextidx != INVALID_PGPROCNO)
{
- PGPROC *proc = &ProcGlobal->allProcs[nextidx];
- PGXACT *pgxact = &ProcGlobal->allPgXact[nextidx];
+ proc = &ProcGlobal->allProcs[nextidx];
+ pgxact = &ProcGlobal->allPgXact[nextidx];
/*
* Overflowed transactions should not use group XID status update
@@ -546,7 +547,7 @@ TransactionGroupUpdateXidStatus(TransactionId xid, XidStatus status,
*/
while (wakeidx != INVALID_PGPROCNO)
{
- PGPROC *proc = &ProcGlobal->allProcs[wakeidx];
+ proc = &ProcGlobal->allProcs[wakeidx];
wakeidx = pg_atomic_read_u32(&proc->clogGroupNext);
pg_atomic_write_u32(&proc->clogGroupNext, INVALID_PGPROCNO);
diff --git a/src/backend/access/transam/multixact.c b/src/backend/access/transam/multixact.c
index 7b2448e05b..6364014fb3 100644
--- a/src/backend/access/transam/multixact.c
+++ b/src/backend/access/transam/multixact.c
@@ -1589,10 +1589,10 @@ mXactCachePut(MultiXactId multi, int nmembers, MultiXactMember *members)
qsort(entry->members, nmembers, sizeof(MultiXactMember), mxactMemberComparator);
dlist_push_head(&MXactCache, &entry->node);
+ pfree(entry);
if (MXactCacheMembers++ >= MAX_CACHE_ENTRIES)
{
dlist_node *node;
- mXactCacheEnt *entry;
node = dlist_tail_node(&MXactCache);
dlist_delete(node);
diff --git a/src/backend/catalog/heap.c b/src/backend/catalog/heap.c
index 8404904710..98f6bc6954 100644
--- a/src/backend/catalog/heap.c
+++ b/src/backend/catalog/heap.c
@@ -1878,19 +1878,18 @@ heap_drop_with_catalog(Oid relid)
*/
if (rel->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
{
- Relation rel;
- HeapTuple tuple;
+ Relation reltp;
- rel = table_open(ForeignTableRelationId, RowExclusiveLock);
+ reltp = table_open(ForeignTableRelationId, RowExclusiveLock);
tuple = SearchSysCache1(FOREIGNTABLEREL, ObjectIdGetDatum(relid));
if (!HeapTupleIsValid(tuple))
elog(ERROR, "cache lookup failed for foreign table %u", relid);
- CatalogTupleDelete(rel, &tuple->t_self);
+ CatalogTupleDelete(reltp, &tuple->t_self);
ReleaseSysCache(tuple);
- table_close(rel, RowExclusiveLock);
+ table_close(reltp, RowExclusiveLock);
}
/*
diff --git a/src/backend/catalog/namespace.c b/src/backend/catalog/namespace.c
index e251f5a9fd..9c997c1b29 100644
--- a/src/backend/catalog/namespace.c
+++ b/src/backend/catalog/namespace.c
@@ -1103,7 +1103,6 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
{
/* Re-order the argument types into call's logical order */
Oid *proargtypes = procform->proargtypes.values;
- int i;
for (i = 0; i < pronargs; i++)
newResult->args[i] = proargtypes[argnumbers[i]];
@@ -1116,8 +1115,6 @@ FuncnameGetCandidates(List *names, int nargs, List *argnames,
}
if (variadic)
{
- int i;
-
newResult->nvargs = effective_nargs - pronargs + 1;
/* Expand variadic argument into N copies of element type */
for (i = pronargs - 1; i < effective_nargs; i++)
diff --git a/src/backend/catalog/objectaddress.c b/src/backend/catalog/objectaddress.c
index ae3002bb42..f357d8fa8d 100644
--- a/src/backend/catalog/objectaddress.c
+++ b/src/backend/catalog/objectaddress.c
@@ -2000,7 +2000,7 @@ pg_get_object_address(PG_FUNCTION_ARGS)
ObjectAddress addr;
TupleDesc tupdesc;
Datum values[3];
- bool nulls[3];
+ bool nullsp[3];
HeapTuple htup;
Relation relation;
@@ -2254,11 +2254,11 @@ pg_get_object_address(PG_FUNCTION_ARGS)
values[0] = ObjectIdGetDatum(addr.classId);
values[1] = ObjectIdGetDatum(addr.objectId);
values[2] = Int32GetDatum(addr.objectSubId);
- nulls[0] = false;
- nulls[1] = false;
- nulls[2] = false;
+ nullsp[0] = false;
+ nullsp[1] = false;
+ nullsp[2] = false;
- htup = heap_form_tuple(tupdesc, values, nulls);
+ htup = heap_form_tuple(tupdesc, values, nullsp);
PG_RETURN_DATUM(HeapTupleGetDatum(htup));
}
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index 94411b5008..a65286330d 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -115,7 +115,6 @@ compute_return_type(TypeName *returnType, Oid languageOid,
{
char *typnam = TypeNameToString(returnType);
Oid namespaceId;
- AclResult aclresult;
char *typname;
ObjectAddress address;
@@ -1000,8 +999,6 @@ CreateFunction(ParseState *pstate, CreateFunctionStmt *stmt)
if (languageStruct->lanpltrusted)
{
/* if trusted language, need USAGE privilege */
- AclResult aclresult;
-
aclresult = pg_language_aclcheck(languageOid, GetUserId(), ACL_USAGE);
if (aclresult != ACLCHECK_OK)
aclcheck_error(aclresult, OBJECT_LANGUAGE,
diff --git a/src/backend/commands/indexcmds.c b/src/backend/commands/indexcmds.c
index 374e2d0efe..ea6361ae96 100644
--- a/src/backend/commands/indexcmds.c
+++ b/src/backend/commands/indexcmds.c
@@ -813,7 +813,6 @@ DefineIndex(Oid relationId,
if (partitioned && (stmt->unique || stmt->primary))
{
PartitionKey key = rel->rd_partkey;
- int i;
/*
* A partitioned table can have unique indexes, as long as all the
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index fbf11c86aa..5b8c6bf0ba 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -60,7 +60,7 @@ parse_publication_options(List *options,
bool *publish_delete,
bool *publish_truncate)
{
- ListCell *lc;
+ ListCell *l;
*publish_given = false;
@@ -71,9 +71,9 @@ parse_publication_options(List *options,
*publish_truncate = true;
/* Parse options */
- foreach(lc, options)
+ foreach(l, options)
{
- DefElem *defel = (DefElem *) lfirst(lc);
+ DefElem *defel = (DefElem *) lfirst(l);
if (strcmp(defel->defname, "publish") == 0)
{
