On /*----- comments - Mailing list pgsql-hackers

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)