Horton

Thread

Commits

  1. Mark ReplicationSlotCtl as PGDLLIMPORT.

  2. Mark assorted variables PGDLLIMPORT.

  3. contrib/amcheck needs RecentGlobalXmin to be PGDLLIMPORT'ified.

  1. Add PGDLLIMPORT lines to some variables

    Brian Cloutier <brian@citusdata.com> — 2017-11-16T22:59:17Z

    Hello hackers,

I'm porting Citus to Windows and found that we use some variables which PG
doesn't export; here is a patch which adds PGDLLIMPORT declarations to
those variables. This is unfortunately required on Windows for extensions
to be able to use those variables, and appears to already have been done to
quite a few other variables.

  2. Re: Add PGDLLIMPORT lines to some variables

    Pavel Stehule <pavel.stehule@gmail.com> — 2017-11-17T06:27:56Z

    Hi

2017-11-16 23:59 GMT+01:00 Brian Cloutier <brian@citusdata.com>:

> Hello hackers,
>
> I'm porting Citus to Windows and found that we use some variables which PG
> doesn't export; here is a patch which adds PGDLLIMPORT declarations to
> those variables. This is unfortunately required on Windows for extensions
> to be able to use those variables, and appears to already have been done to
> quite a few other variables.
>

please, append session_timezone to your list

Regards

Pavel

  3. Re: Add PGDLLIMPORT lines to some variables

    Brian Cloutier <brian@citusdata.com> — 2017-11-20T19:58:44Z

    > please, append session_timezone to your list

Here's a v2 patch which also includes session_timezone.

Separately, is this the kind of thing which is eligible for backporting
into the next releases of 9.6 and 10?

On Thu, Nov 16, 2017 at 10:27 PM, Pavel Stehule <pavel.stehule@gmail.com>
wrote:

> Hi
>
> 2017-11-16 23:59 GMT+01:00 Brian Cloutier <brian@citusdata.com>:
>
>> Hello hackers,
>>
>> I'm porting Citus to Windows and found that we use some variables which
>> PG doesn't export; here is a patch which adds PGDLLIMPORT declarations to
>> those variables. This is unfortunately required on Windows for extensions
>> to be able to use those variables, and appears to already have been done to
>> quite a few other variables.
>>
>
> please, append session_timezone to your list
>
> Regards
>
> Pavel
>
>

  4. Re: Add PGDLLIMPORT lines to some variables

    Andres Freund <andres@anarazel.de> — 2017-11-20T20:02:30Z

    Hi,

Please quote properly on postgres mailing lists... We're old school [tm].

On 2017-11-20 11:58:44 -0800, Brian Cloutier wrote:
> > please, append session_timezone to your list
> 
> Here's a v2 patch which also includes session_timezone.
> 
> Separately, is this the kind of thing which is eligible for backporting
> into the next releases of 9.6 and 10?

I don't think we quite have an established protocol for this. I
personally, but I'm biased in this specific case, is that we should
adopt a position that PGDLLIMPORTs should basically backpatched whenever
a credible extension even halfway reasonably requires it. There's no
easy way to get this done by default, and we're so far unwilling to just
slap this onto every variable. So to not further disadvantage people
force dto live in the MS environment, that seems the sanest
solution. It's not like these are high risk.

- Andres

  5. Re: Add PGDLLIMPORT lines to some variables

    Craig Ringer <craig@2ndquadrant.com> — 2017-11-21T01:26:29Z

    On 21 November 2017 at 04:02, Andres Freund <andres@anarazel.de> wrote:

> Hi,
>
> Please quote properly on postgres mailing lists... We're old school [tm].
>
> On 2017-11-20 11:58:44 -0800, Brian Cloutier wrote:
> > > please, append session_timezone to your list
> >
> > Here's a v2 patch which also includes session_timezone.
> >
> > Separately, is this the kind of thing which is eligible for backporting
> > into the next releases of 9.6 and 10?
>
> I don't think we quite have an established protocol for this. I
> personally, but I'm biased in this specific case, is that we should
> adopt a position that PGDLLIMPORTs should basically backpatched whenever
> a credible extension even halfway reasonably requires it. There's no
> easy way to get this done by default, and we're so far unwilling to just
> slap this onto every variable. So to not further disadvantage people
> force dto live in the MS environment, that seems the sanest
> solution. It's not like these are high risk.
>
>
+1

I'm all in favour of backports to the last supported release or the first
one the variable appeared in.



-- 
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  6. Re: Add PGDLLIMPORT lines to some variables

    Brian Cloutier <brian@citusdata.com> — 2017-11-28T20:00:05Z

    Sorry, I'm new to pg-hackers, so I'm not sure what the next step is.

Do I submit this to commitfest?

When submitting, do I submit multiple changes, one per branch this should
be packported to?

  7. Re: Add PGDLLIMPORT lines to some variables

    Michael Paquier <michael.paquier@gmail.com> — 2017-11-28T22:51:36Z

    On Wed, Nov 29, 2017 at 5:00 AM, Brian Cloutier <brian@citusdata.com> wrote:
> Sorry, I'm new to pg-hackers, so I'm not sure what the next step is.
>
> Do I submit this to commitfest?
>
> When submitting, do I submit multiple changes, one per branch this should be
> packported to?

If you want a patch to get reviewed, please register it to the commit
fest? Here are more details:
https://wiki.postgresql.org/wiki/Submitting_a_Patch
Sending as a first step only one patch for HEAD is fine, as discussion
will likely occur while you work on a patch. If the patch is worth
back-patching, usually the committer who picks up the patch would
generate the versions for the back-branches. Help is always
appreciated with version-specific patches once it is clearly decided
that a certain patch merits a backpatch. Note that PGDLLIMPORT does
not have this treatment usually.
-- 
Michael

  8. Re: Add PGDLLIMPORT lines to some variables

    Noah Misch <noah@leadboat.com> — 2017-12-05T01:41:25Z

    On Mon, Nov 20, 2017 at 12:02:30PM -0800, Andres Freund wrote:
> On 2017-11-20 11:58:44 -0800, Brian Cloutier wrote:
> > > please, append session_timezone to your list
> > 
> > Here's a v2 patch which also includes session_timezone.
> > 
> > Separately, is this the kind of thing which is eligible for backporting
> > into the next releases of 9.6 and 10?
> 
> I don't think we quite have an established protocol for this. I
> personally, but I'm biased in this specific case, is that we should
> adopt a position that PGDLLIMPORTs should basically backpatched whenever
> a credible extension even halfway reasonably requires it. There's no
> easy way to get this done by default, and we're so far unwilling to just
> slap this onto every variable. So to not further disadvantage people
> force dto live in the MS environment, that seems the sanest
> solution. It's not like these are high risk.

+1

  9. Re: Add PGDLLIMPORT lines to some variables

    Peter Geoghegan <pg@bowt.ie> — 2017-12-05T01:48:01Z

    .
On Mon, Dec 4, 2017 at 5:41 PM, Noah Misch <noah@leadboat.com> wrote:
>> I don't think we quite have an established protocol for this. I
>> personally, but I'm biased in this specific case, is that we should
>> adopt a position that PGDLLIMPORTs should basically backpatched whenever
>> a credible extension even halfway reasonably requires it. There's no
>> easy way to get this done by default, and we're so far unwilling to just
>> slap this onto every variable. So to not further disadvantage people
>> force dto live in the MS environment, that seems the sanest
>> solution. It's not like these are high risk.
>
> +1

If that's going to be the policy, then I have some requests of my own.
I would like to add some PGDLLIMPORTs to suit the external version of
amcheck (the version that targets earlier versions of Postgres). These
are:

RecentGlobalXmin -- This is only PGDLLIMPORT on Postgres 10+,
following commit 56018bf2. I'd like to get that back to 9.4, although
there is no reason to not include 9.3.

TransactionXmin -- This is needed for the newer heap-matches-index
verification check. Again, I would like this on 9.4+, but 9.3+ works
too.

Note that somebody asked about running it on Windows recently, and on
one other occasion in the past. It does come up.

-- 
Peter Geoghegan

  10. Re: Add PGDLLIMPORT lines to some variables

    Robert Haas <robertmhaas@gmail.com> — 2017-12-05T14:49:10Z

    On Mon, Dec 4, 2017 at 8:48 PM, Peter Geoghegan <pg@bowt.ie> wrote:
> On Mon, Dec 4, 2017 at 5:41 PM, Noah Misch <noah@leadboat.com> wrote:
>>> I don't think we quite have an established protocol for this. I
>>> personally, but I'm biased in this specific case, is that we should
>>> adopt a position that PGDLLIMPORTs should basically backpatched whenever
>>> a credible extension even halfway reasonably requires it. There's no
>>> easy way to get this done by default, and we're so far unwilling to just
>>> slap this onto every variable. So to not further disadvantage people
>>> force dto live in the MS environment, that seems the sanest
>>> solution. It's not like these are high risk.
>>
>> +1
>
> If that's going to be the policy, then I have some requests of my own.
> I would like to add some PGDLLIMPORTs to suit the external version of
> amcheck (the version that targets earlier versions of Postgres). These
> are:
>
> RecentGlobalXmin -- This is only PGDLLIMPORT on Postgres 10+,
> following commit 56018bf2. I'd like to get that back to 9.4, although
> there is no reason to not include 9.3.
>
> TransactionXmin -- This is needed for the newer heap-matches-index
> verification check. Again, I would like this on 9.4+, but 9.3+ works
> too.
>
> Note that somebody asked about running it on Windows recently, and on
> one other occasion in the past. It does come up.

Committed with these additions.  Please check that I haven't messed anything up.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

  11. Re: Add PGDLLIMPORT lines to some variables

    Peter Geoghegan <pg@bowt.ie> — 2017-12-05T17:09:05Z

    On Tue, Dec 5, 2017 at 6:49 AM, Robert Haas <robertmhaas@gmail.com> wrote:
>> RecentGlobalXmin -- This is only PGDLLIMPORT on Postgres 10+,
>> following commit 56018bf2. I'd like to get that back to 9.4, although
>> there is no reason to not include 9.3.
>>
>> TransactionXmin -- This is needed for the newer heap-matches-index
>> verification check. Again, I would like this on 9.4+, but 9.3+ works
>> too.
>>
>> Note that somebody asked about running it on Windows recently, and on
>> one other occasion in the past. It does come up.
>
> Committed with these additions.  Please check that I haven't messed anything up.

Thanks, but you modified RecentGlobalDataXmin, not RecentGlobalXmin.
Can you fix it, please?

-- 
Peter Geoghegan

  12. Re: Add PGDLLIMPORT lines to some variables

    Peter Geoghegan <pg@bowt.ie> — 2017-12-05T17:13:53Z

    On Tue, Dec 5, 2017 at 9:09 AM, Peter Geoghegan <pg@bowt.ie> wrote:
>> Committed with these additions.  Please check that I haven't messed anything up.
>
> Thanks, but you modified RecentGlobalDataXmin, not RecentGlobalXmin.
> Can you fix it, please?

I was looking at the wrong branch. Forget it.

Thanks
-- 
Peter Geoghegan

  13. Re: Add PGDLLIMPORT lines to some variables

    Craig Ringer <craig@2ndquadrant.com> — 2017-12-07T05:47:32Z

    On 5 December 2017 at 22:49, Robert Haas <robertmhaas@gmail.com> wrote:


>
> Committed with these additions.  Please check that I haven't messed
> anything up.
>
>
Looks good to me.

For the record the commit is

commit c572599c65bfe0387563233faabecd2845073538
Author: Robert Haas <rhaas@postgresql.org>
Date:   Tue Dec 5 09:23:57 2017 -0500

    Mark assorted variables PGDLLIMPORT.

    This makes life easier for extension authors who wish to support
    Windows.

    Brian Cloutier, slightly amended by me.

    Discussion:
http://postgr.es/m/CAJCy68fscdNhmzFPS4kyO00CADkvXvEa-28H-OtENk-pa2OTWw@mail.gmail.com

plus back branches.

I was going to pipe up here to add ReplicationSlotCtl to the list.
Otherwise the only way to access slot information is via the SPI and
pg_stat_replication_slots, which isn't super fun. And it's not like
ReplicationSlotCtl is any more internal than MyReplicationSlot.

I missed the boat on your commit, but ... please?

Patches attached. MyReplicationSlot was only made PGDLLIMPORT in 9.6, so
there's one for 9.4/9.5 and one for 9.6, 10, and master. Personally I don't
care about 9.4/9.5 in the slightest for this, but that's where c572599c is
backpatched to.

--
 Craig Ringer                   http://www.2ndQuadrant.com/
 PostgreSQL Development, 24x7 Support, Training & Services

  14. Re: Add PGDLLIMPORT lines to some variables

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-06-13T14:56:06Z

    [ blast-from-the-past department ]

Craig Ringer <craig@2ndquadrant.com> writes:
> On 5 December 2017 at 22:49, Robert Haas <robertmhaas@gmail.com> wrote:
>>     Mark assorted variables PGDLLIMPORT.

> I was going to pipe up here to add ReplicationSlotCtl to the list.
> Otherwise the only way to access slot information is via the SPI and
> pg_stat_replication_slots, which isn't super fun. And it's not like
> ReplicationSlotCtl is any more internal than MyReplicationSlot.

> I missed the boat on your commit, but ... please?

Not sure why this request was ignored, but now that we have another
request for the same thing [1], I've pushed this.

			regards, tom lane

[1] https://postgr.es/m/345138875.20190611151943@cybertec.at