Обсуждение: question regarding policy for patches to out-of-support branches
I was having a discussion regarding out-of-support branches and effort
to keep them building, but could not for the life of me find any actual
documented policy (although I distinctly remember that we do something...).
I did find fleeting references, for example:
8<-----------------------
commit c705646b751e08d584f6eeb098f1ed002aa7b11c
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date: 2022-09-21 13:52:38 -0400
<snip>
Per project policy, this is a candidate for back-patching into
out-of-support branches: it suppresses annoying compiler warnings
but changes no behavior. Hence, back-patch all the way to 9.2.
8<-----------------------
and on its related thread:
8<-----------------------
However, I think that that would *not* be fit material for
back-patching into out-of-support branches, since our policy
for them is "no behavioral changes".
8<-----------------------
Is the policy written down somewhere, or is it only project lore? In
either case, what is the actual policy?
Thanks,
--
Joe Conway
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes: > I was having a discussion regarding out-of-support branches and effort > to keep them building, but could not for the life of me find any actual > documented policy (although I distinctly remember that we do something...). > Is the policy written down somewhere, or is it only project lore? In > either case, what is the actual policy? I believe our policy was set in this thread: https://www.postgresql.org/message-id/flat/2923349.1634942313%40sss.pgh.pa.us and you're right that it hasn't really been memorialized anywhere else. I'm not sure where would be appropriate. Anyway, what I think the policy is: * Out-of-support versions back to (currently) 9.2 are still to be kept buildable on modern toolchains. * Build failures, regression failures, and easily-fixable compiler warnings are candidates for fixes. * We aren't too excited about code that requires external dependencies (e.g. libpython) though, because those can be moving targets. * Under no circumstances back-patch anything that changes external behavior, as the point of the exercise is to be able to test against the actual behavior of the last releases of these branches. regards, tom lane
On Wed, Jun 5, 2024 at 8:29 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > Joe Conway <mail@joeconway.com> writes: > > I was having a discussion regarding out-of-support branches and effort > > to keep them building, but could not for the life of me find any actual > > documented policy (although I distinctly remember that we do something...). > > Is the policy written down somewhere, or is it only project lore? In > > either case, what is the actual policy? > > I believe our policy was set in this thread: > > https://www.postgresql.org/message-id/flat/2923349.1634942313%40sss.pgh.pa.us > > and you're right that it hasn't really been memorialized anywhere > else. I'm not sure where would be appropriate. Not absolutely sure, but would at least adding a page to PostgreSQL Wiki about this make sense ? --- Hannu
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. People don't read or remember every mailing list discussion forever, and even if they did, how would we enumerate all the policies for the benefit of a newcomer? Maybe this belongs in the documentation, maybe in the wiki, maybe someplace else, but the real issue for me is that policies have to be discoverable by the people who need to adhere to them, and old mailing list discussions aren't. -- Robert Haas EDB: http://www.enterprisedb.com
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...)
On 6/6/24 14:12, Tom Lane wrote: > 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? I added them here with minimal copy editing an no attempt to organize or sort into groups: https://wiki.postgresql.org/wiki/Committing_checklist#Policies If someone has thoughts on how to improve I am happy to make more changes. -- Joe Conway PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Joe Conway <mail@joeconway.com> writes: > On 6/6/24 14:12, Tom Lane wrote: >> 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? > I added them here with minimal copy editing an no attempt to organize or > sort into groups: > https://wiki.postgresql.org/wiki/Committing_checklist#Policies > If someone has thoughts on how to improve I am happy to make more changes. Thanks! I summoned the energy to make a few more improvements, particularly updating stuff that seemed out-of-date. I'm sure there's more that could be added here. regards, tom lane
On Thu, Jun 6, 2024 at 10:04 PM Tom Lane <tgl@sss.pgh.pa.us> wrote: > > I added them here with minimal copy editing an no attempt to organize or > > sort into groups: > > https://wiki.postgresql.org/wiki/Committing_checklist#Policies > > If someone has thoughts on how to improve I am happy to make more changes. > > Thanks! I summoned the energy to make a few more improvements, > particularly updating stuff that seemed out-of-date. I'm sure > there's more that could be added here. This is nice! I wonder if we could interest anyone in creating tooling that could be used to check some of this stuff -- ideally run as part of the regular build process, so that you fail to notice that you did it wrong. Not all of these rules are subject to automatic verification e.g. it's hard to enforce that a change to an out-of-support branch makes no functional change. But an awful lot of them could be, and I would personally be significantly happier and less stressed if I knew that 'ninja && meson test' was going to tell me that I did it wrong before I pushed, instead of finding out afterward and then having to drop everything to go clean it up. -- Robert Haas EDB: http://www.enterprisedb.com