Fix typos and inconsistencies for v16

On Mon, Apr 17, 2023 at 09:00:00PM +0300, Alexander Lakhin wrote:

Hello hackers,

Please consider fixing the following unique words/identifiers introduced in v16:

Well done.

Note that your patches are overlapping:

3 --- a/src/backend/utils/misc/guc.c
2 --- a/src/test/perl/PostgreSQL/Test/BackgroundPsql.pm
2 --- a/src/test/ldap/LdapServer.pm
2 --- a/src/interfaces/libpq/t/004_load_balance_dns.pl
2 --- a/src/backend/utils/adt/acl.c

It'd make sense if the changes to each file were isolated to a single
patch (especially 004_load and acl.c).

-		 * USER SET values are appliciable only for PGC_USERSET parameters. We
+		 * USER SET values are applicable only for PGC_USERSET parameters. We
* use InvalidOid as role in order to evade possible privileges of the

and s/evade/avoid/

+++ b/src/bin/pg_dump/pg_dumpall.c

You missed "boostrap" :)

I independently found 11 of the same typos you did:

1. addresess -> addresses
3. appeneded -> appended
4. appliciable -> applicable
8. containsthe -> �contains the
15. execpt -> except
19. happend -> happened
27. optionn -> option
30. permissons -> permissions
37. remaing -> remaining
42. sentinal -> sentinel
47. varilables -> variables

But hadn't yet convinced myself to start the process of defending each
one of the fixes. Attached some others that I found.

--
Justin

On Tue, 18 Apr 2023 at 06:00, Alexander Lakhin <exclusion@gmail.com> wrote:

Please consider fixing the following unique words/identifiers introduced in v16:

Thanks, I've pushed all of these apart from the following 2.

45. tar_set_error -- remove (obsolete since ebfb814f7)
46. test_tranche_name -- remove (not used, see 006b69fd9)

These didn't quite fit in with the "typo fixes" category of the
commit, so I left them off the commit I just pushed.

Also, maybe OID_MAX should be removed from src/include/postgres_ext.h as it's unused since eb8312a22.

I didn't touch this. It seems like it could be useful for extensions
and client apps even if it's not used in core.

Beside that, this simple script:
for w in $(cat src/tools/pgindent/typedefs.list); do grep -q -P "\b$w\b" -r * --exclude typedefs.list || echo "$w"; done
detects 58 identifiers that don't exist in the source tree anymore (see typedefs.lost attached).
Maybe they should be removed from typedefs.list too.

I didn't touch this either. typedefs.list normally gets some work
done during the pgindent run, which is likely going to happen around
May or June. Maybe you can check back after that's done and make sure
all these unused ones were removed. I'm not sure if the process that's
done for that only finds new ones that are now required or if it
completely generates a new list.

David

On Tue, 18 Apr 2023 at 10:10, Justin Pryzby <pryzby@telsasoft.com> wrote:

-              * USER SET values are appliciable only for PGC_USERSET parameters. We
+              * USER SET values are applicable only for PGC_USERSET parameters. We
* use InvalidOid as role in order to evade possible privileges of the

and s/evade/avoid/

I didn't touch this. You'll need to provide more justification for why
you think it's more correct than what's there. It might not be worth
too much discussion, however.

Attached some others that I found.

Pushed the rest. Thanks

David

David Rowley <dgrowleyml@gmail.com> writes:

On Tue, 18 Apr 2023 at 06:00, Alexander Lakhin <exclusion@gmail.com> wrote:

Also, maybe OID_MAX should be removed from src/include/postgres_ext.h as it's unused since eb8312a22.

I didn't touch this. It seems like it could be useful for extensions
and client apps even if it's not used in core.

Agreed, bad idea. For better or worse, that's part of our client API now.

Beside that, this simple script:
for w in $(cat src/tools/pgindent/typedefs.list); do grep -q -P "\b$w\b" -r * --exclude typedefs.list || echo "$w"; done
detects 58 identifiers that don't exist in the source tree anymore (see typedefs.lost attached).
Maybe they should be removed from typedefs.list too.

I didn't touch this either. typedefs.list normally gets some work
done during the pgindent run, which is likely going to happen around
May or June.

Yeah, it will get refreshed from the buildfarm output [1]https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list pretty soon.
A quick check says that as of today, that refresh would add 81 names
and remove 94. (Seems like a remarkably high number of removals,
but I didn't dig further than counting the diff output.)

regards, tom lane

[1]: https://buildfarm.postgresql.org/cgi-bin/typedefs.pl?show_list

On Tue, Apr 18, 2023 at 02:06:43PM +1200, David Rowley wrote:

On Tue, 18 Apr 2023 at 10:10, Justin Pryzby <pryzby@telsasoft.com> wrote:

-              * USER SET values are appliciable only for PGC_USERSET parameters. We
+              * USER SET values are applicable only for PGC_USERSET parameters. We
* use InvalidOid as role in order to evade possible privileges of the

and s/evade/avoid/

I didn't touch this. You'll need to provide more justification for why
you think it's more correct than what's there.

I'd noticed that it's a substitution/mistake that's been made in the
past. I dug up:

9436041ed848debb3d64fb5fbff6cdb35bc46d04
8e12f4a250d250a89153da2eb9b91c31bb80c483
cd9479af2af25d7fa9bfd24dd4dcf976b360f077
6df7a9698bb036610c1e8c6d375e1be38cb26d5f
911e70207703799605f5a0e8aad9f06cff067c63

It might not be worth too much discussion, however.

+many. I may resend the patch at some later date.

Attached some others that I found.

Pushed the rest. Thanks

Thanks!

--
Justin

Hi Justin and David,

18.04.2023 01:10, Justin Pryzby wrote:

Well done.

Thank you for reviewing!

On Mon, Apr 17, 2023 at 09:00:00PM +0300, Alexander Lakhin wrote:

Hello hackers,

Please consider fixing the following unique words/identifiers introduced in v16:

Note that your patches are overlapping:
...
It'd make sense if the changes to each file were isolated to a single
patch (especially 004_load and acl.c).

I'd hoped that most of the proposed fixes will be accepted, so conflicts due
to skipping of some changes seemed unlikely to me. So if you are not
strongly disagree, I would continue presenting my findings the same way.

...
You missed "boostrap" :)

Yes, that's because "boostrap" was not unique, but my semi-automatic approach
is based on `uniq -u`, so I'm sure that there are typos that can't be found
this way.

But hadn't yet convinced myself to start the process of defending each
one of the fixes. Attached some others that I found.

Yeah, those are good catches too, but e. g. "privilges" is not new in v16,
so it's fallen out of my "hot errors" category. If we're going to fix not so
hot ones too now, please look at the similar list for v15+ (596b5af1d..HEAD).

1. abbrevated -> abbreviated
2. ArchiveModeRequested -> ArchiveRecoveryRequested
3. BufFileOpenShared -> BufFileOpenFileSet // see dcac5e7ac
4. check_publication_columns -> pub_collist_contains_invalid_column // see note 1
5. configuation -> configuration
6. copyAclUserName -> dequoteAclUserName // see 0c9d84427
7. EndWalRecovery -> FinishWalRecovery
8. HaveRegisteredOrActiveSnapshots -> HaveRegisteredOrActiveSnapshot
9. idiosyncracies -> idiosyncrasies
10. iif -> iff

11. initpriv -> initprivs
12. inserted_destrel -> insert_destrel
13. Intialize -> Initialize
14. invtrans -> invtransfn
15. isolation-level -> isolation level
16. lefthasheqoperator -> left_hasheqoperator + righthasheqoperator -> right_hasheqoperator
17. LRQ_NO_IO -> LRQ_NEXT_NO_IO
18. minRecovery point -> minRecoveryPoint
19. multidimensional-aware -> multidimension-aware // sync with gistbuild.c
20. ParalleVacuumState -> ParallelVacuumState

21. PgStatShm_Stat*Entry -> PgStatShared_* // see note 2
22. plpython_call_handler -> plpython3_call_handler // see 9b7e24a2c
23. pulications -> publications
24. ReadCheckPointRecord -> ReadCheckpointRecord
25. relkkinds -> relkinds
26. separare -> separate // though perhaps it's not the most suitable word here
27. setup_formatted_log_time -> get_formatted_log_time // see ac7c80758
28. SPI_abort -> SPI_rollback
29. ssup_datum_int32_compare -> ssup_datum_int32_cmp
30. ssup_datum_signed_compare -> ssup_datum_signed_cmp

31. ssup_datum_unsigned_compare -> ssup_datum_unsigned_cmp
32. SUBSCRITPION -> SUBSCRIPTION
33. tabelspaces -> tablespaces
34. table_state_not_ready -> table_states_not_ready
35. underling -> underlying
36. WalRecoveryResult -> EndOfWalRecoveryInfo

Also, I'd like to note that the following entities/references are orphaned now,
so maybe some of them could be removed too:
1. gen-rtab (in pgcrypto/Makefile) // orphaned since db7d1a7b0
2. pgstat_temp_directory // was left by b3abca681 for v15, but maybe it's time to remove it for v16
3. pgstat_write_statsfiles (in valgrind.supp)
4. quote_system_arg (in vcregress.pl) // unused since d2a2ce418
5. standard_initdb (in vcregress.pl) // unused since 322becb60
/* though maybe vcregress.pl will be removed completely soon */
6. int                     pstat;                  /* mcrypt uses it */ (in contrib/pgcrypto/px.h)
/* "mcrypt" became unique after abe81ee08, support for libmcrypt was removed at 2005
(3cc866123) */

Note 1. A check that was located in check_publication_columns() in
v13-0003-Add-column-filtering-to-logical-replication.patch [1]/messages/by-id/202112302021.ca7ihogysgh3@alvherre.pgsql,
can be found in pub_collist_contains_invalid_column() now (see also [2]/messages/by-id/CAA4eK1K5pkrPT9z5TByUPptExian5c18g6GnfNf9Cr97QdPbjw@mail.gmail.com).

Note 2. The inconsistency appeared in [3]/messages/by-id/20220404041516.cctrvpadhuriawlq@alap3.anarazel.de,
v67-0007-pgstat-store-statistics-in-shared-memory.patch was correct in
this aspect.

18.04.2023 04:35, David Rowley wrote:

Please consider fixing the following unique words/identifiers introduced in v16:

Thanks, I've pushed all of these apart from the following 2.

Thank you!

[1]: /messages/by-id/202112302021.ca7ihogysgh3@alvherre.pgsql
[2]: /messages/by-id/CAA4eK1K5pkrPT9z5TByUPptExian5c18g6GnfNf9Cr97QdPbjw@mail.gmail.com
[3]: /messages/by-id/20220404041516.cctrvpadhuriawlq@alap3.anarazel.de

Best regards,
Alexander

Justin Pryzby <pryzby@telsasoft.com> writes:

On Tue, Apr 18, 2023 at 02:06:43PM +1200, David Rowley wrote:

On Tue, 18 Apr 2023 at 10:10, Justin Pryzby <pryzby@telsasoft.com> wrote:

and s/evade/avoid/

I didn't touch this. You'll need to provide more justification for why
you think it's more correct than what's there.

I'd noticed that it's a substitution/mistake that's been made in the
past.

"Evade" doesn't seem like le mot juste there; it's got negative
connotations. But the code around it is just horrible. Some offenses:

* No documentation in the function header comment of what the
usersetArray parameter is or does. Which is bad enough in itself,
but what the parameter actually does is falsify the header comment's
principal claim that the passed context is what is used. So I don't
find that omission acceptable.

* Non-obvious, and quite unnecessary, dependency on the isnull variable
having been left in a particular state by previous code.

* For me, at least, it'd read better if the if/else arms were swapped,
allowing removal of the negation in the if-condition and bringing
the code this comment comments on closer to said comment.

As for the comment text, maybe say

* If the value was USER SET, then apply it at PGC_USERSET context
* rather than the caller-supplied context, to prevent any more-restricted
* GUCs being set. Also pass InvalidOid for the role, to ensure any
* special privileges of the current user aren't applied.

I hesitate to go look at the rest of this commit, but I guess somebody
had better.

regards, tom lane

On Wed, 19 Apr 2023 at 07:00, Alexander Lakhin <exclusion@gmail.com> wrote:

please look at the similar list for v15+ (596b5af1d..HEAD).

I've now pushed most of these but didn't include the following ones:

3. BufFileOpenShared -> BufFileOpenFileSet // see dcac5e7ac

Maybe I need to spend longer, but I just didn't believe the command
that claimed that "BufFiles opened using BufFileOpenFileSet() are
read-only by definition". Looking at the code for that, it seems to
depend on if O_RDONLY is included in the mode flags.

19. multidimensional-aware -> multidimension-aware // sync with gistbuild.c

I didn't change this as I didn't think it was an improvement. I'd
probably have written "multidimensionally aware", but I didn't feel
strongly enough to want to change it.

David

Hi David,

21.04.2023 01:49, David Rowley wrote:

On Wed, 19 Apr 2023 at 07:00, Alexander Lakhin <exclusion@gmail.com> wrote:

please look at the similar list for v15+ (596b5af1d..HEAD).

I've now pushed most of these but didn't include the following ones:

Thank you!

3. BufFileOpenShared -> BufFileOpenFileSet // see dcac5e7ac

Maybe I need to spend longer, but I just didn't believe the command
that claimed that "BufFiles opened using BufFileOpenFileSet() are
read-only by definition". Looking at the code for that, it seems to
depend on if O_RDONLY is included in the mode flags.

I've found the following explanation for that:
1) As of dcac5e7ac~1 function ltsConcatWorkerTapes() contained:
...
                file = BufFileOpenShared(fileset, filename, O_RDONLY);
...
         * The only thing that currently prevents writing to the leader tape from
         * working is the fact that BufFiles opened using BufFileOpenShared() are
         * read-only by definition, but that could be changed if it seemed
...

2) A patch [1]/messages/by-id/91284957-3cb2-944e-5f14-5c2ff86b49fa@iki.fi (0001-Refactor-LogicalTapeSet-LogicalTape-interface.patch), which eventually resulted in c4649cce3, initially started
with the change:

-ltsConcatWorkerTapes(LogicalTapeSet *lts, TapeShare *shared,
...
-     * working is the fact that BufFiles opened using BufFileOpenShared() are
...
+LogicalTapeImport(LogicalTapeSet *lts, int worker, TapeShare *shared)
...
+    file = BufFileOpenShared(lts->fileset, filename, O_RDONLY);
...
+     * the fact that BufFiles opened using BufFileOpenShared() are read-only

3) The commit dcac5e7ac (pushed 2021-08-30) renamed the function
BufFileOpenShared() to BufFileOpenFileSet() and changed the comment:
...
          * The only thing that currently prevents writing to the leader tape from
-        * working is the fact that BufFiles opened using BufFileOpenShared() are
+        * working is the fact that BufFiles opened using BufFileOpenFileSet() are
          * read-only by definition, but that could be changed if it seemed
...

4) The commit c4649cce3 (pushed 2021-10-18) removed the comment referencing
BufFileOpenFileSet() and added that somewhat distant comment
referencing BufFileOpenShared():
$ git show c4649cce3 src/backend/utils/sort/logtape.c | grep 'BufFiles opened'
-        * working is the fact that BufFiles opened using BufFileOpenFileSet() are
+        * the fact that BufFiles opened using BufFileOpenShared() are read-only

So I still believe that the "BufFileOpenShared -> BufFileOpenFileSet" change
is correct and that comment can be read now as referencing to the line:
    file = BufFileOpenFileSet(&lts->fileset->fs, filename, O_RDONLY, false);
in LogicalTapeImport(). Although it could be improved, for sure.

Please look at the following two bunches for v14+ and v13+ (split to ease
back-patching if needed). Having processed them, I've reached the state that
could be considered "clean" ([2]/messages/by-id/5da8e325-c665-da95-21e0-c8a99ea61fbf@gmail.com, [3]/messages/by-id/CALDaNm0ni+GAOe4+fbXiOxNrVudajMYmhJFtXGX-zBPoN8ixhw@mail.gmail.com); at least I don't see how to detect
yet more errors of this class in dozens, so it's my last run for now (though I
have several entities left, which I couldn't find replacements for).

v14+:
1. AsyncCtl -> NotifyCtl // renamed in 5da14938f
2. ATExecConstrRecurse -> ATExecAlterConstrRecurse
3. attlocal -> attislocal
4. before_shmem_access -> before_shmem_exit
5. bodys -> bodies
6. can_getnextslot_tidrange -> scan_getnextslot_tidrange
7. DISABLE_ATOMICS -> HAVE_ATOMICS
8. FETCH_H -> REWIND_SOURCE_H
9. filed -> field
10. find_minmax_aggs_walker -> can_minmax_aggs // renamed in 0a2bc5d61e

11. GroupExprInfo ->  GroupVarInfo //// a4d75c86b
12. LD_DEAD -> LP_DEAD
13. libpq-trace.c -> fe-trace.c
14. lowerItem -> lowestItem //// bb437f995
15. has_privs -> has_privs_of_role
16. heap_hot_prune_opt -> heap_page_prune_opt
17. MAX_CONVERSION_LENGTH -> MAX_CONVERSION_INPUT_LENGTH //// ea1b99a66
18. MAX_FLUSH_BUFFERS -> MAX_WRITEALL_BUFFERS // renamed in dee663f78
19. myscheam -> myschema // doc/ -- maybe should be backpatched
20. pgbestat_beinit -> pgstat_beinit

21. pgWALUsage -> pgWalUsage
22. point-in-time-recovery -> point-in-time recovery
23. PQnotify -> PGnotify
24. QUERYJUBLE_H -> QUERYJUMBLE_H
25. rd_partdesc_nodetach -> rd_partdesc_nodetached
26. ReadNewTransactionid -> GetNewTransactionId
27. RelationBuildDescr -> RelationBuildDesc
28. SnapBuildCommittedTxn -> SnapBuildCommitTxn // see DecodeCommit()
29. subscription_rel -> pg_subscription_rel
30. tap_rep -> tab_rep

31. total_heap_blks -> heap_blks_total
32. tuple_cids -> tuplecids
33. WatchLatch -> WaitLatch
34. WriteAll -> SimpleLruWriteAll
35. PageIsPrunable -- remove // that define and the PageIsPrunable() check above were removed in dc7420c2c

Candidates for removal:
BARRIER_SHOULD_CHECK //unused since a3ed4d1ef
EXE_EXT // unused since f06b1c598
get_toast_for // unused since 860593ec3
SizeOfCommitTsSet // unused since 08aa89b32

v13+:
1. agg_init_trans_check -> agg_trans
2. agg_strict_trans_check -> agg_trans
3. amopclassopts -> amoptsprocnum //// since 911e70207
4. CommitTSBuffer -> CommitTsBuffer // the inconsistency exists since 5da14938f; maybe this change should be backpatched
5. gist_intbig_ops -> gist__intbig_ops
6. gist_int_ops -> gist__int_ops
7. laftleft -> lastleft
8. lc_message -> locale_name // in accordance with the search_locale_enum() description
9. leftype -> lefttype
10. mksafefunc -> mkfunc // see 1f474d299

11. openSegment -> segment_open // in accordance with the WALRead() description
12. parse_util.c -> parse_utilcmd.c
13. process_innerer_partition -> process_inner_partition
14. read_spilled_tuple -> hashagg_batch_read
15. SortGroupNode -> SortGroupClause
16. SWITCH_WAL -> XLOG_SWITCH
17. tts_attr -> ttc_attr
18. tts_oldvalues -> ttc_oldvalues
19. tts_oldisnull -> ttc_oldisnull
20. tts_rel -> ttc_rel

21. taget -> target
22. WALSnd -> WalSnd
23. XLogRoutine -> XLogReaderRoutine

Candidates for removal:
endterm // see 60c90c16c -- Use xreflabel attributes instead of endterm attributes ...
package_tarname // not used since introduction in 1933ae629
my $clearpass = "FooBaR1"; // unreferenced since b846091fd

[1]: /messages/by-id/91284957-3cb2-944e-5f14-5c2ff86b49fa@iki.fi (0001-Refactor-LogicalTapeSet-LogicalTape-interface.patch)
(0001-Refactor-LogicalTapeSet-LogicalTape-interface.patch)

[2]: /messages/by-id/5da8e325-c665-da95-21e0-c8a99ea61fbf@gmail.com
[3]: /messages/by-id/CALDaNm0ni+GAOe4+fbXiOxNrVudajMYmhJFtXGX-zBPoN8ixhw@mail.gmail.com

Best regards,
Alexander

On Fri, Apr 21, 2023 at 12:00:00PM +0300, Alexander Lakhin wrote:

Please look at the following two bunches for v14+ and v13+ (split to ease
back-patching if needed). Having processed them, I've reached the state that
could be considered "clean" ([2], [3]); at least I don't see how to detect
yet more errors of this class in dozens, so it's my last run for now (though I
have several entities left, which I couldn't find replacements for).

This was hanging around, and I had some time, so I have looked at the
whole. One of the only two user-visible change was in the docs for
pg_amcheck, so I have applied that first as of 6fd8ae6 and backpatched
it down to 14.

Now, for the remaining 59..

1. agg_init_trans_check -> agg_trans
2. agg_strict_trans_check -> agg_trans

    /*
     * pergroup = &aggstate->all_pergroups
-    * [op->d.agg_strict_trans_check.setoff]
-    * [op->d.agg_init_trans_check.transno];
+    * [op->d.agg_trans.setoff]
+    * [op->d.agg_trans.transno];
     */
Honestly, while incorrect, I have no idea what this comment means ;)

4. CommitTSBuffer -> CommitTsBuffer // the inconsistency exists since 5da14938f; maybe this change should be backpatched

Yes, we'd better backpatch that. I agree that it seems more sensible
here to switch the compiled value rather than what the docs have been
using for years. Perhaps somebody has a different opinion?

The others were OK and in line with the discussion of upthread, so
applied.
--
Michael

On Tue, May 02, 2023 at 12:26:31PM +0900, Michael Paquier wrote:

On Fri, Apr 21, 2023 at 12:00:00PM +0300, Alexander Lakhin wrote:

4. CommitTSBuffer -> CommitTsBuffer // the inconsistency exists since 5da14938f; maybe this change should be backpatched

Yes, we'd better backpatch that. I agree that it seems more sensible
here to switch the compiled value rather than what the docs have been
using for years. Perhaps somebody has a different opinion?

Hearing nothing, I have now applied this part down to 13, on time for
the next minor release.
--
Michael