Thread: Fix typos and inconsistencies for v16
Hello hackers, Please consider fixing the following unique words/identifiers introduced in v16: 1. addresess -> addresses 2. adminstrator -> administrator // the same typo found in src/backend/po/id.po, but perhaps it should be fixed via pgsql-translators 3. appeneded -> appended 4. appliciable -> applicable 5. BackroundPsql -> BackgroundPsql 6. binaies -> binaries 7. compresion -> compression 8. containsthe -> contains the 9. contextes -> contexts 10. deparseAnalyzeTuplesSql -> deparseAnalyzeInfoSql // that function was renamed in 57d11ef0 11. DO_LARGE_OJECT_DATA -> DO_LARGE_OBJECT_DATA 12. doesnt't -> doesn't 13. dst_perminfo -> dst_perminfos 14. eror -> error 15. execpt -> except 16. forech -> foreach 17. GetResultRelCheckAsUser -> ExecGetResultRelCheckAsUser 18. GUCS -> GUCs 19. happend -> happened 20. immitated -> imitated 21. insert_xid -> tuple_xid // see bfcf1b348 22. ldap_add -> ldapadd_file 23. ldapbindpassw -> ldapbindpasswd 24. MemoryChunkSetExternal -> MemoryChunkSetHdrMaskExternal 25. non-encyrpted -> non-encrypted 26. --no-process_main -> --no-process-main 27. optionn -> option 28. Othewise -> Otherwise 29. parellel -> parallel 30. permissons -> permissions 31. pg_pwrite_zeroes -> pg_pwrite_zeros 32. pg_writev -> pg_pwritev 33. possbile -> possible 34. pqsymlink -> pgsymlink 35. PG_GET_WAL_FPI_BLOCK_COLS -> PG_GET_WAL_BLOCK_INFO_COLS 36. RangeVarCallbackOwnsTable -> RangeVarCallbackMaintainsTable // see 60684dd83 37. remaing -> remaining 38. ResourceOwnerForgetBufferIOs -> ResourceOwnerForgetBufferIO 39. RMGRDESC_UTILS_H -> RMGRDESC_UTILS_H_ // or may be the other way 40. rolenamehash -> rolename_hash 41. ROLERECURSE_SETROLe -> ROLERECURSE_SETROLE 42. sentinal -> sentinel 43. smgzerorextend -> smgrzeroextend 44. stacktoodeep -> rstacktoodeep // an excessive character was deleted with db4f21e4a? 45. tar_set_error -- remove (obsolete since ebfb814f7) 46. test_tranche_name -- remove (not used, see 006b69fd9) 47. varilables -> variables 48. xid_commit_status -> xmin_commit_status Also, maybe OID_MAX should be removed from src/include/postgres_ext.h as it's unused since eb8312a22. 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. Best regards, Alexander
Attachment
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
Attachment
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] 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:
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],
can be found in pub_collist_contains_invalid_column() now (see also [2]).
Note 2. The inconsistency appeared in [3],
v67-0007-pgstat-store-statistics-in-shared-memory.patch was correct in
this aspect.
18.04.2023 04:35, David Rowley wrote:
Thank you!Please consider fixing the following unique words/identifiers introduced in v16:Thanks, I've pushed all of these apart from the following 2.
[1] https://www.postgresql.org/message-id/202112302021.ca7ihogysgh3%40alvherre.pgsql
[2] https://www.postgresql.org/message-id/CAA4eK1K5pkrPT9z5TByUPptExian5c18g6GnfNf9Cr97QdPbjw%40mail.gmail.com
[3] https://www.postgresql.org/message-id/20220404041516.cctrvpadhuriawlq%40alap3.anarazel.de
Best regards,
Alexander
Attachment
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], 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(<s->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], [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). 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] https://www.postgresql.org/message-id/91284957-3cb2-944e-5f14-5c2ff86b49fa%40iki.fi (0001-Refactor-LogicalTapeSet-LogicalTape-interface.patch) [2] https://www.postgresql.org/message-id/flat/5da8e325-c665-da95-21e0-c8a99ea61fbf%40gmail.com [3] https://www.postgresql.org/message-id/flat/CALDaNm0ni%2BGAOe4%2BfbXiOxNrVudajMYmhJFtXGX-zBPoN8ixhw%40mail.gmail.com Best regards, Alexander
Attachment
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
Attachment
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