Hi Steve and slony-hackers,
	Sure, I see how that simplifies maintainability going forward, and it’s fine for us to use a version check disable option.
	Thus far I’ve noted this change, the use of snprintf with the pg_home setting and the removal of copyright headers. I have another deadline this week so I’ll look to provide updated patches in the week following.

	Tom    (


On 2/3/18, 4:15 PM, "Steve Singer" <steve at ssinger.info> wrote:

    On Tue, 23 Jan 2018, Tignor, Tom wrote:
    
    >    compatibility_v2.2.patch
    >
    >    My understanding of this patch is so that you slon for any 2.2.x version to
    >    be able to connect and replicate against a backend database with any other
    >    2.2.y version (where x and y can be, but are not required to be equal).
    >
    > ttignor – Yes, that’s the intention and I believe the exact effect. 
    > Specifically, we’re upgrading our slony1-dependent services from 
    > slony1-2.2.4 to slony1-2.2.6. The upgrade process we’ve verified requires 
    > a mixed mode environment for a limited period of time (a few days). I 
    > chose “2.2.x” arbitrarily. We could achieve what we need with more 
    > specific criteria, or a config option, or a combination of the two. 
    > --------
    
    How do you feel about instead having an option to disable the slony version 
    check.
    
    My concern with saying all 2.2.x versions are compatible or even that 
    certain minor/patch versions are compatible is the cross version testing 
    load it puts on slony maintainers and how it might restrict things in the 
    future.
    
    If instead we just provide a option to disable the version check users can 
    use this option as they see fit.
    
    
    
    >
    > A few things on the specific patch
    >
    >    +++ slony1-2.2.6/src/slon/dbutils.c     2017-12-22 08:08:33.027322631 -0500
    >    @@ -5,6 +5,7 @@
    >       *
    >       *     Copyright (c) 2003-2009, PostgreSQL Global Development Group
    >       *     Author: Jan Wieck, Afilias USA INC.
    >    + *      Copyright (C) 2017 - Akamai Technologies, Inc
    >       *
    >       *
    >       * ----------------------------------------------------------------------
    >    @@ -418,7 +419,8 @@
    >
    >    We don't normally list contributors in the copyright section. The copyright
    >    to PostgreSQL global development group is intended to cover all
    >    contributors.
    >
    > ttignor – I’ve gone a few rounds on this with our opensource group. As I understand the issues, there is a choice to either maintain or assign away the Akamai copyright for the code I wrote.  Per our previous discussion, we’re maintaining the Akamai copyright. This is accomplished by header comments and/or some meta-data in the patch. If the header comment is a problem, then the patch meta-data becomes essential. If slony1-hackers can agree on exactly what copyright changes are needed, I can take them back to Akamai opensource.
    > --------
    >
    > So for the pg_home patch.
    > …
    >  #else
    > +       char *pgHome = getenv("PG_HOME");
    > +       if (pgHome) {
    > +         strncpy(share_path, pgHome, MAXPGPATH-1);
    > +         share_path[MAXPGPATH-1] = '\0';
    > +         strncat(share_path, "/share", MAXPGPATH-1-strlen(pgHome));
    > +       } else {
    >         strcpy(share_path, PGSHARE);
    > +       }
    >
    >
    > The above code only gets compiled in if PGPORT is not defined/present at
    > build time.  Is that your intention,  or do you want PG_HOME to take
    > precendence even if pgport is present?
    >
    > snprintf(share_path,"%s/share",pgHome,MAXPGPATH-1);
    >
    > Would the above code be clearer? (I haven't tested/tried to compile above)
    > but trying to do this in one line?
    >
    > ttignor – Reviewing the changes, the HAVE_PGPORT clause is marked with a comment “We need to find a share directory like PostgreSQL.”. That seemed like something I shouldn’t override with a customization. Re: snprintf, that seems like a good improvement. I see snprintf can also truncate output, so I’ll need to account for that as well with a rewrite.
    > --------
    >
    > 	More thoughts from Steve or others? Keep them coming.
    >
    > 	Tom    (
    >
    >
    >
    >
    >
    >
    >