Re: question regarding policy for patches to out-of-support branches - Mailing list pgsql-hackers

Robert Haas <robertmhaas@gmail.com> writes:
> On Thu, Jun 6, 2024 at 4:25 AM Hannu Krosing <hannuk@google.com> wrote:
>> Not absolutely sure, but would at least adding a page to PostgreSQL
>> Wiki about this make sense ?

> I feel like we need to do something. Tom says this is a policy, and
> he's made that comment before about other things, but the fact that
> they're not memorialized anywhere is a huge problem, IMHO.

I didn't say it wasn't ;-)

ISTM we have two basic choices: wiki page, or new SGML docs section.
In the short term I'd lean to a wiki page.  It'd be reasonable for

https://wiki.postgresql.org/wiki/Committing_checklist

to link to it (and maybe the existing section there about release
freezes would be more apropos on a "Project policies" page?  Not
sure.)

To get a sense of how much of a problem we have, I grepped the git
history for comments mentioning project policies.  Ignoring ones
that are really talking about very localized issues, what I found
is attached.  It seems like it's little enough that a single wiki
page with subsections ought to be enough.  I'm not super handy with
editing the wiki, plus I'm only firing on one cylinder today (seem
to have acquired a head cold at pgconf), so maybe somebody else
would like to draft something?

            regards, tom lane


    This was submitted as a security issue, but the security team
    has been unable to identify any consequence worse than a null
    pointer dereference (from trying to access rd_tableam methods
    that the relation no longer has).  Therefore, in accordance
    with our usual policy, it's not security material and should
    just be fixed as a routine bug.
    (this is probably material for security-team-private documentation)

    All backend-side variables should be marked with PGDLLIMPORT, as per
    policy introduced in 8ec569479f.

    Project policy is to not leave global objects behind after a regress
    test run.  This was found as a result of the development of a patch
    to make pg_regress detect such leftovers automatically, which in the
    end was withdrawn due to issues with parallel runs.

    Per project policy, transient roles created by regression test cases
    should be named "regress_something", to reduce the risks of running
    such cases against installed servers.  And no such role should ever
    be left behind after running a test.

    Per project policy that we want to keep recently-out-of-support
    branches buildable on modern systems, back-patch all the way to 9.2.

    This back-patches commit 9ff47ea41 into out-of-support branches,
    pursuant to newly-established project policy.  The point is to
    suppress scary-looking warnings so that people building these
    branches needn't expend brain cells verifying that it's safe
    to ignore the warnings.

    Tweak detail and hint messages to be consistent with project policy
    (this should reference message style guide in SGML docs)

    Doc: update testing recipe in src/test/perl/README.
    The previous text didn't provide any clear explanation of our policy
    around TAP test portability.
    (should just reference that README as a guide for writing TAP tests)

    "typename" is a C++ keyword, so pg_upgrade.h fails to compile in C++.
    Fortunately, there seems no likely reason for somebody to need to
    do that.  Nonetheless, it's project policy that all .h files should
    pass cpluspluscheck, so rename the argument to fix that.

    Commit a6417078 established a new project policy around OID assignment:
    new patches are encouraged to choose a random OID in the 8000..9999
    range when a manually-assigned OID is required (if multiple OIDs are
    required, a consecutive block of OIDs starting from the random point
    should be used).  Catalog entries added by committed patches that use
    OIDs from this "unstable" range are renumbered after feature freeze.
    (this should reference bki.sgml)

    libpq failed to ignore Windows-style newlines in connection service files.
    This normally wasn't a problem on Windows itself, because fgets() would
    convert \r\n to just \n.  But if libpq were running inside a program that
    changes the default fopen mode to binary, it would see the \r's and think
    they were data.  In any case, it's project policy to ignore \r in text
    files unconditionally, because people sometimes try to use files with
    DOS-style newlines on Unix machines, where the C library won't hide that
    from us.

    However, project policy since parallel query came in is that all plan
    node types should have outfuncs/readfuncs support, so this is clearly
    an oversight that should be repaired.
    (Probably moot now, given auto generation of these functions.)

    We have a project policy that every .c file should start by including
    postgres.h, postgres_fe.h, or c.h as appropriate; and then there is no
    need for any .h file to explicitly include any of these.  (The core
    reason for this policy is to make it easy to verify that pg_config_os.h
    is included before any system headers such as <stdio.h>; without that,
    we have portability issues on some platforms due to variation in largefile
    options across different modules in the backend.  Also, if .h files were
    responsible for choosing which of these key headers to include, .h files
    that need to be includable in either frontend or backend compiles would be
    in trouble.)

    Per project policy, all system and library headers need to be declared
    in the backend code after "postgres.h" and before the internal headers,
    but 4035cd5 broke this policy when adding support for LZ4 in
    wal_compression.

    We have a not-terribly-thoroughly-enforced-yet project policy that internal
    errors with SQLSTATE XX000 (ie, plain elog) should not be triggerable from
    SQL.  record_in, domain_in, and PL validator functions all failed to meet
    this standard, because they threw plain elog("cache lookup failed for XXX")
    errors on bad OIDs, and those are all invokable from SQL.

    It's against project policy to use elog() for user-facing errors, or to
    omit an errcode() selection for errors that aren't supposed to be "can't
    happen" cases.  Fix all the violations of this policy that result in
    ERRCODE_INTERNAL_ERROR log entries during the standard regression tests,
    as errors that can reliably be triggered from SQL surely should be
    considered user-facing.

    Commit e5e11c8cc added a bunch of EXPLAIN statements without COSTS OFF
    to the regression tests.  This is contrary to project policy since it
    results in unnecessary platform dependencies in the output (it's just
    luck that we didn't get buildfarm failures from it).

    In type_sanity, check I/O functions of built-in types are not volatile.
    We have a project policy that I/O functions must not be volatile, as per
    commit aab353a60b95aadc00f81da0c6d99bde696c4b75, but we weren't doing
    anything to enforce that.
    This test as such will only protect us against future errors in built-in
    data types.  To catch the same error in contrib or third-party types,
    perhaps we should make CREATE TYPE complain?  But that's a separate
    issue from enforcing the policy for built-in types.
    (this is moot now for built-in types, but not for contrib...)