Re: Fix typos and inconsistencies for v16

From: Alexander Lakhin <exclusion(at)gmail(dot)com>
To: David Rowley <dgrowleyml(at)gmail(dot)com>
Cc: Justin Pryzby <pryzby(at)telsasoft(dot)com>, pgsql-hackers(at)postgresql(dot)org
Subject: Re: Fix typos and inconsistencies for v16
Date: 2023-04-21 09:00:00
Message-ID: e8c38840-596a-83d6-bd8d-cebc51111572@gmail.com
Views: Raw Message | Whole Thread | Download mbox | Resend email
Thread:
Lists: pgsql-hackers

Hi David,

21.04.2023 01:49, David Rowley wrote:
> On Wed, 19 Apr 2023 at 07:00, Alexander Lakhin <exclusion(at)gmail(dot)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(&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], [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 Content-Type Size
typo-fixes-for-v14+.1-10.patch text/x-patch 5.3 KB
typo-fixes-for-v14+.11-20.patch text/x-patch 6.6 KB
typo-fixes-for-v14+.21-30.patch text/x-patch 6.4 KB
typo-fixes-for-v14+.31-35.patch text/x-patch 2.9 KB
typo-fixes-for-v13+.1-10.patch text/x-patch 4.9 KB
typo-fixes-for-v13+.11-20.patch text/x-patch 4.7 KB
typo-fixes-for-v13+.21-23.patch text/x-patch 1.7 KB

In response to

Responses

Browse pgsql-hackers by date

  From Date Subject
Next Message Amit Kapila 2023-04-21 09:16:51 Re: Support logical replication of DDLs
Previous Message Amul Sul 2023-04-21 08:58:06 Re: New committers: Nathan Bossart, Amit Langote, Masahiko Sawada