Thread: Add PGDLLIMPORT lines to some variables

Add PGDLLIMPORT lines to some variables

From
Brian Cloutier
Date:
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.
Attachment

Re: Add PGDLLIMPORT lines to some variables

From
Pavel Stehule
Date:
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

Re: Add PGDLLIMPORT lines to some variables

From
Brian Cloutier
Date:
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


Attachment

Re: Add PGDLLIMPORT lines to some variables

From
Andres Freund
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Craig Ringer
Date:
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

Re: Add PGDLLIMPORT lines to some variables

From
Brian Cloutier
Date:
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?

Re: Add PGDLLIMPORT lines to some variables

From
Michael Paquier
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Noah Misch
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Peter Geoghegan
Date:
.
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


Re: Add PGDLLIMPORT lines to some variables

From
Robert Haas
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Peter Geoghegan
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Peter Geoghegan
Date:
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


Re: Add PGDLLIMPORT lines to some variables

From
Craig Ringer
Date:
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.
    

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
Attachment

Re: Add PGDLLIMPORT lines to some variables

From
Tom Lane
Date:
[ 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