Add PGDLLIMPORT lines to some variables

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
13 messages Options
Reply | Threaded
Open this post in threaded view
|

Add PGDLLIMPORT lines to some variables

Brian Cloutier
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.

pgdllimport-v1.patch (8K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Pavel Stehule
Hi

2017-11-16 23:59 GMT+01:00 Brian Cloutier <[hidden email]>:
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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Brian Cloutier
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 <[hidden email]> wrote:
Hi

2017-11-16 23:59 GMT+01:00 Brian Cloutier <[hidden email]>:
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



pgdllimport-v2.patch (9K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Craig Ringer-3
On 21 November 2017 at 04:02, Andres Freund <[hidden email]> 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
Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Brian Cloutier
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?
Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Michael Paquier
On Wed, Nov 29, 2017 at 5:00 AM, Brian Cloutier <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Noah Misch-2
In reply to this post by Andres Freund
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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Peter Geoghegan-4
.
On Mon, Dec 4, 2017 at 5:41 PM, Noah Misch <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Robert Haas
On Mon, Dec 4, 2017 at 8:48 PM, Peter Geoghegan <[hidden email]> wrote:

> On Mon, Dec 4, 2017 at 5:41 PM, Noah Misch <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Peter Geoghegan-4
On Tue, Dec 5, 2017 at 6:49 AM, Robert Haas <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Peter Geoghegan-4
On Tue, Dec 5, 2017 at 9:09 AM, Peter Geoghegan <[hidden email]> 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

Reply | Threaded
Open this post in threaded view
|

Re: Add PGDLLIMPORT lines to some variables

Craig Ringer-3
In reply to this post by Robert Haas
On 5 December 2017 at 22:49, Robert Haas <[hidden email]> 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 <[hidden email]>
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

94-95-v1-0001-Make-MyReplicationSlot-and-ReplicationSlotCtl-PGD.patch (1K) Download Attachment
96-10-master-v1-0001-Mark-ReplicationSlotCtl-as-PGDLLIMPORT.patch (1K) Download Attachment
Previous Thread Next Thread