Add PGDLLIMPORT lines to some variables

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

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,

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

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

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?

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

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

.
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

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

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

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

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:
/messages/by-id/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

[ 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]/messages/by-id/345138875.20190611151943@cybertec.at, I've pushed this.

regards, tom lane

[1]: /messages/by-id/345138875.20190611151943@cybertec.at