Обсуждение: On /*----- comments
I spotted this comment in walsender.c: > /*------- > * When reading from a historic timeline, and there is a timeline switch > * [.. long comment omitted ...] > * portion of the old segment is copied to the new file. ------- > */ Note the bogus dashes at the end. This was introduced in commit 0dc8ead463, which moved and reformatted the comment. It's supposed to end the pgindent-guard at the beginning of the comment like this: > /*------- > * When reading from a historic timeline, and there is a timeline switch > * [.. long comment omitted ...] > * portion of the old segment is copied to the new file. > *------- > */ But that got me wondering, do we care about those end-guards? pgindent doesn't need them. We already have a bunch of comments that don't have the end-guard, for example: analyze.c: > /*------ > translator: %s is a SQL row locking clause such as FOR UPDATE */ gistproc.c: > /*---- > * The goal is to form a left and right interval, so that every entry > * interval is contained by either left or right interval (or both). > * > * For example, with the intervals (0,1), (1,3), (2,3), (2,4): > * > * 0 1 2 3 4 > * +-+ > * +---+ > * +-+ > * +---+ > * > * The left and right intervals are of the form (0,a) and (b,4). > * We first consider splits where b is the lower bound of an entry. > * We iterate through all entries, and for each b, calculate the > * smallest possible a. Then we consider splits where a is the > * upper bound of an entry, and for each a, calculate the greatest > * possible b. > * > * In the above example, the first loop would consider splits: > * b=0: (0,1)-(0,4) > * b=1: (0,1)-(1,4) > * b=2: (0,3)-(2,4) > * > * And the second loop: > * a=1: (0,1)-(1,4) > * a=3: (0,3)-(2,4) > * a=4: (0,4)-(2,4) > */ predicate.c: > /*---------- > * The SLRU is no longer needed. Truncate to head before we set head > * invalid. > * > * XXX: It's possible that the SLRU is not needed again until XID > * wrap-around has happened, so that the segment containing headPage > * that we leave behind will appear to be new again. In that case it > * won't be removed until XID horizon advances enough to make it > * current again. > * > * XXX: This should happen in vac_truncate_clog(), not in checkpoints. > * Consider this scenario, starting from a system with no in-progress > * transactions and VACUUM FREEZE having maximized oldestXact: > * - Start a SERIALIZABLE transaction. > * - Start, finish, and summarize a SERIALIZABLE transaction, creating > * one SLRU page. > * - Consume XIDs to reach xidStopLimit. > * - Finish all transactions. Due to the long-running SERIALIZABLE > * transaction, earlier checkpoints did not touch headPage. The > * next checkpoint will change it, but that checkpoint happens after > * the end of the scenario. > * - VACUUM to advance XID limits. > * - Consume ~2M XIDs, crossing the former xidWrapLimit. > * - Start, finish, and summarize a SERIALIZABLE transaction. > * SerialAdd() declines to create the targetPage, because headPage > * is not regarded as in the past relative to that targetPage. The > * transaction instigating the summarize fails in > * SimpleLruReadPage(). > */ indexcmds.c: > /*----- > * Now we have all the indexes we want to process in indexIds. > * > * The phases now are: > * > * 1. create new indexes in the catalog > * 2. build new indexes > * 3. let new indexes catch up with tuples inserted in the meantime > * 4. swap index names > * 5. mark old indexes as dead > * 6. drop old indexes > * > * We process each phase for all indexes before moving to the next phase, > * for efficiency. > */ Except for the translator comments, I think those others forgot about the end-guards by accident. But they look just as nice to me. It's probably not worth the code churn to remove them from existing comments, but how about we stop requiring them in new code, and update the pgindent README accordingly? -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> Except for the translator comments, I think those others forgot about
> the end-guards by accident. But they look just as nice to me. It's
> probably not worth the code churn to remove them from existing comments,
> but how about we stop requiring them in new code, and update the
> pgindent README accordingly?
Seems reasonable; the trailing dashes eat a line without adding much.
Should we also provide specific guidance about how many leading dashes
to use for this? I vaguely recall that pgindent might only need one,
but I think using somewhere around 5 to 10 looks better.
regards, tom lane
> On 30 Jun 2023, at 17:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: > Seems reasonable; the trailing dashes eat a line without adding much. +1 > Should we also provide specific guidance about how many leading dashes > to use for this? I vaguely recall that pgindent might only need one, > but I think using somewhere around 5 to 10 looks better. There are ~50 different lenghts used when looking at block comments from line 2 (to avoid the file header comment) and onwards in files, the ones with 10 or more occurrences are: 145 /*---------- 78 /*------ 76 /*------------------------------------------------------------------------- 37 /*---------------------------------------------------------- 29 /*------------------------ 23 /*---------------------------------------------------------------- 22 /*-------------------- 21 /*---- 15 /*--------------------------------------------------------------------- 14 /*-- 13 /*------------------------------------------------------- 13 /*--- 12 /*---------------------- 10 leading dashes is the clear winner so recommending that for new/edited comments seem like a good way to reduce churn. Looking at line 1 comments for fun shows pretty strong consistency: 1611 /*------------------------------------------------------------------------- 22 /*-------------------------------------------------------------------------- 18 /*------------------------------------------------------------------------ 13 /*-------------------------------------------------------------------- 7 /*--------------------------------------------------------------------------- 4 /*----------------------------------------------------------------------- 4 /*---------------------------------------------------------------------- 1 /*-------------------------- plpy_util.h being the only one that sticks out. -- Daniel Gustafsson
On 03/07/2023 11:48, Daniel Gustafsson wrote: >> On 30 Jun 2023, at 17:22, Tom Lane <tgl@sss.pgh.pa.us> wrote: > >> Seems reasonable; the trailing dashes eat a line without adding much. > > +1 Pushed a patch to remove the end-guard from the example in the pgindent README. And fixed the bogus end-guard in walsender.c. >> Should we also provide specific guidance about how many leading dashes >> to use for this? I vaguely recall that pgindent might only need one, >> but I think using somewhere around 5 to 10 looks better. > > There are ~50 different lenghts used when looking at block comments from line 2 > (to avoid the file header comment) and onwards in files, the ones with 10 or > more occurrences are: > > 145 /*---------- > 78 /*------ > 76 /*------------------------------------------------------------------------- > 37 /*---------------------------------------------------------- > 29 /*------------------------ > 23 /*---------------------------------------------------------------- > 22 /*-------------------- > 21 /*---- > 15 /*--------------------------------------------------------------------- > 14 /*-- > 13 /*------------------------------------------------------- > 13 /*--- > 12 /*---------------------- > > 10 leading dashes is the clear winner so recommending that for new/edited > comments seem like a good way to reduce churn. The example in the pgindent README also uses 10 dashes. I'm not sure there is a universal best length. It depends on the comment what looks best. The very long ones in particular would not look good on comments in a deeply indented block. So I think the status quo is fine. > Looking at line 1 comments for fun shows pretty strong consistency: > > 1611 /*------------------------------------------------------------------------- > 22 /*-------------------------------------------------------------------------- > 18 /*------------------------------------------------------------------------ > 13 /*-------------------------------------------------------------------- > 7 /*--------------------------------------------------------------------------- > 4 /*----------------------------------------------------------------------- > 4 /*---------------------------------------------------------------------- > 1 /*-------------------------- > > plpy_util.h being the only one that sticks out. I don't see any reason for the variance in these. Seems accidental.. -- Heikki Linnakangas Neon (https://neon.tech)
Heikki Linnakangas <hlinnaka@iki.fi> writes:
> On 03/07/2023 11:48, Daniel Gustafsson wrote:
> On 30 Jun 2023, at 17:22, Tom Lane <tgl@sss.pgh.pa.us> wrote:
>>> Seems reasonable; the trailing dashes eat a line without adding much.
>> +1
> Pushed a patch to remove the end-guard from the example in the pgindent
> README. And fixed the bogus end-guard in walsender.c.
I don't see any actual push?
> I'm not sure there is a universal best length. It depends on the comment
> what looks best. The very long ones in particular would not look good on
> comments in a deeply indented block. So I think the status quo is fine.
OK, no strong feeling about that here.
regards, tom lane
On 04/07/2023 21:36, Tom Lane wrote: > Heikki Linnakangas <hlinnaka@iki.fi> writes: >> Pushed a patch to remove the end-guard from the example in the pgindent >> README. And fixed the bogus end-guard in walsender.c. > > I don't see any actual push? Forgot it after all. Pushed now, thanks. -- Heikki Linnakangas Neon (https://neon.tech)