Support multiple PostgreSQL versions

[pgaudit]

Support multiple PostgreSQL versions

[dwsteele]

Hi Adrian,

This was my original intent but turned out not to be workable. I can solve the issues in the C code with #ifdef but that won't work for the regression tests which are not compatible across versions. That being the case I decided to maintain the branches without version #ifdef like the core project does it.

In any case packages for extensions are generally Postgres version specific in my experience. If there were multiple versions of Postgres installed, where would you install a generic extension?

[disco-stu]

Hi David,

wouldn't it be possible to provide a alternative expected/pgaudit.out (pgaudit_1.out)? I've run some tests using alternative files and it seems to work. Did I miss something?

Cheers,
Adrian

[dwsteele]

How would that work in a build from PG source or PGXS?

[disco-stu]

The regression tests do multiple diffs against the various [_digit].out files within the expected directory. E.g.: If pgaudit.out doesn't match results but pgaudit_1.out does the test is passed [1].

One project I know which uses this feature is pg_repack [2].

[1] https://www.postgresql.org/docs/9.6/static/regress-variant.html
[2] https://github.com/reorg/pg_repack/tree/master/regress/expected

[dwsteele]

That's very interesting - I was not aware. However, I'm still not convinced it's a good idea to have pgaudit in flux within a Postgres version. Stability is very important for this module and generally only bug fixes will be back-ported. The inclusion of the log_client setting was a notable exception.

I'll discuss with some colleagues and see what they think.

[sfrost]

Adrian,

I believe the better approach here would be to use different source packages for pgAudit, similar to how PostgreSQL is packaged. In particular, there should probably be a "pgAudit-1.0" source package, which then produces the "postgresql-9.5-pgaudit-1.0" package and a "pgAudit-1.1" source package, which produces the "postgresql-9.6-pgaudit-1.1" package.

Thanks!

[mbanck]

Different source packages look like a lot of overkill to me; Debian does it for major software like GTK, Postgres etc., but a random (if great) Postgres extension? There's dozens of them, I don't see how pgaudit is any different.

[sfrost]

Michael,

Different source packages are necessary when there are different sources trees. That is the case with pgAudit- there is a different source tree for each PG major version and will continue to be moving forward as we maintain each branch in a manner similar to how PostgreSQL major versions are maintained. The packaging does not and should not drive how the project manages its source.

Thanks!

[ivoras]

A similar question, adding it here because it may not warrant a separate open issue: I'm trying to build pgaudit for postgresql 9.3 (I know it's old, but silly corporate policies stop me from upgrading as long as it's officially supported by upstream at postgresql.org), and it fails at including the tcop/deparse_utility.h header. Is this something which can be relatively easily #ifdef-ed?

[dwsteele]

Compiling this version of pgaudit for anything less than 9.5 is not going to work. Not only are there the event triggers, but the entire call stack is based around a memory callback that was introduced in 9.5. There is a version that works on 9.3 (https://github.com/2ndQuadrant/pgaudit), but it has fewer features.

[dwsteele]

Closing this since I hope it is resolved. To be clear, no further changes are planned for the 9.5 and 9.6 back branches unless there are bugs. New development, if any, will be strictly for 10.0 on a new branch.

[mbanck]

@sfrost, a 1-line C-code change (pgaudit.c:738) is not "different source trees" in any normal sense of the word. Granted, there are also changes in the testsuite, but the change in sql/pgaudit.sql does not look 9.6-specific to me, couldn't that be harmonized? As for the expected output, see @disco-stu's comment.

AFAICT this is the whole diff between the 9.5 and 9.6 branches, which is hardly a huge maintenance-burden (at least so far):

REL9_5_STABLE...REL9_6_STABLE

[mbanck]

I'm using the following patch for a preliminary Debian packaging:

https://people.debian.org/~mbanck/pgaudit/support_9.5.patch

That is just the expected/pgaudit.out from the REL9_5_STABLE branch copied over as expected/pgaudit_1.out and the following code-level change:

--- pgaudit-1.1.0.orig/pgaudit.c
+++ pgaudit-1.1.0/pgaudit.c
@@ -36,6 +36,10 @@
 #include "utils/syscache.h"
 #include "utils/timestamp.h"
 
+#if (PG_VERSION_NUM >= 90500 && PG_VERSION_NUM < 90600)
+#define LOG_SERVER_ONLY COMMERROR
+#endif
+
 PG_MODULE_MAGIC;
 
 void _PG_init(void);

I.e. the opposite of https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=66229ac0040cf1e0f5b9d72271aa9feaf3b3a37e

[sfrost]

Michael,

We will be maintaining an independent branch for each major version of PostgreSQL, matching exactly how PostgreSQL is maintained. While I understand that you might wish to pull them together into a since branch for Debian packaging, that isn't how upstream will be maintaining them and will make things confusing and complicated, unnecessarily, for anyone reviewing the upstream source tree or the packaging.

I don't recommend deviating from how the upstream repository is being handled. We will not be back-patching features into earlier versions, we will back-patch bugs, possibly in different ways for different major versions, and we will only be testing the code from the upstream repository. We will be updating for PG10 over the next few months and that will be another branch with possibly more changes in it which would make it much more difficult to maintain a single branch which attempts to have features only for PG10 and only bug-fixes for stable releases. There is a reason that the PG project does not try to maintain one branch for all major versions and we agree with that reasoning and will be using it for pgAudit as well, particularly as it is quite closely tied to PG due to how the extension works and what it does.

Thanks!

Stephen

[df7cb]

Heya,
this "new source package per PG version" idea isn't going to fly. It would be a huge maintenance effort for apt.postgresql.org.
TBH, I don't see why you can't do it like any other PG extension out there - throw in a few #if PG_VERSION_NUM >= 100000 and be done. It's not like the your whole codebase would be affected by it - @mbanck's 9.6-in-9.5 patch was a one-liner, and reading 067cb6a, about 4 #if should do the job for 10 as well.
The comparison with PG upstream development doesn't hold here. We are not asking you to provide #ifdefs that would hide new features or the like, we are just asking for a way to compile HEAD against more than one PG release. If you insist on stable releases, maintain a "1.1" branch and push fixes there, but that doesn't mean that 1.1 or 1.2 need to target only one PG version.
Again, every other PG extension including the really huge PostGIS does it that way. Just follow suit.
Thanks,
Christoph

[dwsteele]

Hi Cristoph,

This scheme was chosen to ensure maximum stability for the back branches. They are fairly static at the moment but this won't always be true. We felt it was better to split them from the beginning rather than have to do it later and make a mess.

In any case, they have been like this for two years so changing them now would be a problem for those who are already packaging pgAudit, which is quite a few organizations at this point.

Thanks,
-David