Обсуждение: PostmasterPid not marked with PGDLLIMPORT
Hi all, While hacking a background worker for Windows/Linux that is sending signals to the Postmaster depending on the state of the server where Postgres is running (particularly after a certain size threshold is reached on the partition of PGDATA SIGINT is sent to PostmasterPid to have it stop cleanly), I have noticed that PostmasterPid is not marked as PGDLLIMPORT in miscadmin.h, so I cannot use the field directly in this background worker for Windows... Bypassing this problem can be done by parsing postmaster.pid but adding this extra logic is really annoying as this requires a one-line change upstream... Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD and back-branches? Regards, -- Michael
Вложения
On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote:
Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD
and back-branches?
--
On Wed, Jun 1, 2016 at 12:06 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote: >> Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD >> and back-branches? > > Sounds sensible to me. I don't really want to set a precedent that we'll back-patch PGDLLIMPORT markings every time somebody needs a new symbol for some extension they are writing, but I don't mind changing this in master. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 1, 2016 at 12:06 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
> On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote:
>> Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD
>> and back-branches?
>
> Sounds sensible to me.
+1
I don't really want to set a precedent that we'll back-patch
PGDLLIMPORT markings every time somebody needs a new symbol for some
extension they are writing [...]
-1
David J.
Robert Haas wrote: > On Wed, Jun 1, 2016 at 12:06 AM, Craig Ringer <craig@2ndquadrant.com> wrote: > > On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote: > >> Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD > >> and back-branches? > > > > Sounds sensible to me. > > I don't really want to set a precedent that we'll back-patch > PGDLLIMPORT markings every time somebody needs a new symbol for some > extension they are writing, but I don't mind changing this in master. I wonder why is that -- just to reduce the commit load? I don't think this kind of change is likely to break anything, is it? -- Álvaro Herrera http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
On Wed, Jun 1, 2016 at 12:24 PM, Alvaro Herrera <alvherre@2ndquadrant.com> wrote: > Robert Haas wrote: >> On Wed, Jun 1, 2016 at 12:06 AM, Craig Ringer <craig@2ndquadrant.com> wrote: >> > On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote: >> >> Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD >> >> and back-branches? >> > >> > Sounds sensible to me. >> >> I don't really want to set a precedent that we'll back-patch >> PGDLLIMPORT markings every time somebody needs a new symbol for some >> extension they are writing, but I don't mind changing this in master. > > I wonder why is that -- just to reduce the commit load? I don't think > this kind of change is likely to break anything, is it? Probably not, but yes, I do want to reduce the commit load. I also think that we essentially have a contract with our users to limit what we back-patch to critical bug fixes and security fixes. When we don't do that, people start asking to have individual fixes cherry-picked instead of just upgrading, and that's not good. We may know that such changes are low-risk, but that doesn't mean everyone else does. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 1, 2016 at 12:24 PM, Alvaro Herrera
<alvherre@2ndquadrant.com> wrote:
> Robert Haas wrote:
>> On Wed, Jun 1, 2016 at 12:06 AM, Craig Ringer <craig@2ndquadrant.com> wrote:
>> > On 1 June 2016 at 11:48, Michael Paquier <michael.paquier@gmail.com> wrote:
>> >> Could it be possible to mark PostmasterPid with PGDLLIMPORT on HEAD
>> >> and back-branches?
>> >
>> > Sounds sensible to me.
>>
>> I don't really want to set a precedent that we'll back-patch
>> PGDLLIMPORT markings every time somebody needs a new symbol for some
>> extension they are writing, but I don't mind changing this in master.
>
> I wonder why is that -- just to reduce the commit load? I don't think
> this kind of change is likely to break anything, is it?
Probably not, but yes, I do want to reduce the commit load. I also
think that we essentially have a contract with our users to limit what
we back-patch to critical bug fixes and security fixes. When we don't
do that, people start asking to have individual fixes cherry-picked
instead of just upgrading, and that's not good. We may know that such
changes are low-risk, but that doesn't mean everyone else does.
Are there two concerns here? One, that people will think we are back-patching stuff and destabilizing back-branches, and two, that people will see increase back-patching and therefore make unreasonable requests of us to which we dislike saying "no". The later doesn't seem likely, and I'd say you can't stop people from having badly formed opinions and that our track record on back-patching decisions is excellent.
We want third-party tools
to support our prior releases and if miss making one of our features available to Windows because of a missing PGDLLIMPORT that's on our heads and should be fixed. If a user equates that to "please batch-patch jsonb to 9.3 because I don't want to upgrade" I'm not going to feel much guilt saying "that's different, ain't gonna happen". Informed people will understand the purpose of the back-patch and until I start hearing a vocal uninformed person start griping I'd rather give the community the benefit of the doubt.
David J.
> On Wed, Jun 1, 2016 at 5:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Probably not, but yes, I do want to reduce the commit load. I also
>> think that we essentially have a contract with our users to limit what
>> we back-patch to critical bug fixes and security fixes. When we don't
>> do that, people start asking to have individual fixes cherry-picked
>> instead of just upgrading, and that's not good. We may know that such
>> changes are low-risk, but that doesn't mean everyone else does.
I suggest that there's a more principled reason for refusing a back-patch
here, which is that we don't back-patch new features, only bug fixes.
This request is certainly not a bug fix. It's in support of a new feature
--- and one that's not even ours, but a third-party extension.
Considering that said extension isn't finished yet, it's hard to guess
whether this will be the only blocking factor preventing it from working
with older versions, but it might well be that there are other problems.
Also, even if it would work, the author would be reduced to saying things
like "it will work in 9.4, if it's 9.4.9 or later". Keeping track of that
level of detail is no fun either for the author or for users. It'd be
a lot simpler all around to just say "my spiffy new extension requires 9.6
or later".
In short, I'd vote for putting this change in HEAD, but I see no need to
back-patch.
regards, tom lane
> On Wed, Jun 1, 2016 at 5:04 PM, Robert Haas <robertmhaas@gmail.com> wrote:
>> Probably not, but yes, I do want to reduce the commit load. I also
>> think that we essentially have a contract with our users to limit what
>> we back-patch to critical bug fixes and security fixes. When we don't
>> do that, people start asking to have individual fixes cherry-picked
>> instead of just upgrading, and that's not good. We may know that such
>> changes are low-risk, but that doesn't mean everyone else does.
I suggest that there's a more principled reason for refusing a back-patch
here, which is that we don't back-patch new features, only bug fixes.
This request is certainly not a bug fix. It's in support of a new feature
--- and one that's not even ours, but a third-party extension.
Maybe I don't understand PGDLLEXPORT...
The PostgreSQL function/feature in question is already in place and can be accessed by someone using Linux or other unix-like variant. But it cannot be access by our Window's users because we failed to add a PGDLLEXPORT somewhere. If it is our goal to treat Windows and Linux/Unix equally then that discrepancy is on its face a bug. The fact we don't catch these until some third-party points it out doesn't make it any less a bug.
Considering that said extension isn't finished yet, it's hard to guess
whether this will be the only blocking factor preventing it from working
with older versions, but it might well be that there are other problems.
From my prior point the reason someone wants to use the unexposed but documented public API shouldn't matter.
Also, even if it would work, the author would be reduced to saying things
like "it will work in 9.4, if it's 9.4.9 or later". Keeping track of that
level of detail is no fun either for the author or for users.
This seems hollow. "It works on the latest version of all officially supported PostgreSQL releases" is a reasonable policy for a third-party application to take. That it might work on older releases would be a bonus for some new users.
It'd be
a lot simpler all around to just say "my spiffy new extension requires 9.6
or later".
And it makes the available user base considerably smaller. A small change to get the other 80% of the users is something to take seriously.
In short, I'd vote for putting this change in HEAD, but I see no need to
back-patch.
I see a need for back-patching and no technical reason why it cannot be done - easily.
David J.
On Thu, Jun 2, 2016 at 6:40 AM, Tom Lane <tgl@sss.pgh.pa.us> wrote: > I suggest that there's a more principled reason for refusing a back-patch > here, which is that we don't back-patch new features, only bug fixes. > This request is certainly not a bug fix. It's in support of a new feature > --- and one that's not even ours, but a third-party extension. Yes, that's not a bug fix. I agree on that. > Considering that said extension isn't finished yet, it's hard to guess > whether this will be the only blocking factor preventing it from working > with older versions, but it might well be that there are other problems. > Also, even if it would work, the author would be reduced to saying things > like "it will work in 9.4, if it's 9.4.9 or later". Keeping track of that > level of detail is no fun either for the author or for users. The extension in this case is for 9.4, for a product yet to be released that is now on 9.4.8, so I don't care much about the support grid here to be honest. > It'd be a lot simpler all around to just say "my spiffy new extension requires 9.6 > or later". Well, that's the only factor as far as I saw that prevented me to use this extension on Windows. But I won't fight your might regarding a backpatch, wearing the burden of a custom patch applied to miscadmin.h for REL9_4_STABLE is not huge: this code never changes. > In short, I'd vote for putting this change in HEAD, but I see no need to > back-patch. OK, fine for me. -- Michael
On Wed, Jun 1, 2016 at 5:59 PM, David G. Johnston <david.g.johnston@gmail.com> wrote: > Maybe I don't understand PGDLLEXPORT... We're talking about PGDLLIMPORT. > The PostgreSQL function/feature in question is already in place and can be > accessed by someone using Linux or other unix-like variant. But it cannot > be access by our Window's users because we failed to add a PGDLLEXPORT > somewhere. If it is our goal to treat Windows and Linux/Unix equally then > that discrepancy is on its face a bug. The fact we don't catch these until > some third-party points it out doesn't make it any less a bug. If we had a policy of putting PGDLLIMPORT on everything, I'd agree with you, but we clearly don't. Something's only a bug if we intended A but accidentally got B. If we intended and got A and somebody doesn't like that, that's not a bug; that's a difference of opinion. I personally feel that we should sprinkle PGDLLIMPORT markings onto a lot more things, but Tom Lane has opposed that at every turn. I hope we'll change our policy about that someday, but that's a different question from whether such changes should be back-patched. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 1, 2016 at 7:39 PM, Michael Paquier <michael.paquier@gmail.com> wrote: >> In short, I'd vote for putting this change in HEAD, but I see no need to >> back-patch. > > OK, fine for me. Done. -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
On Wed, Jun 1, 2016 at 5:59 PM, David G. Johnston
<david.g.johnston@gmail.com> wrote:
> Maybe I don't understand PGDLLEXPORT...
We're talking about PGDLLIMPORT.
Typo, was thinking "we export this for others to consume"...
> The PostgreSQL function/feature in question is already in place and can be
> accessed by someone using Linux or other unix-like variant. But it cannot
> be access by our Window's users because we failed to add a PGDLLEXPORT
> somewhere. If it is our goal to treat Windows and Linux/Unix equally then
> that discrepancy is on its face a bug. The fact we don't catch these until
> some third-party points it out doesn't make it any less a bug.
If we had a policy of putting PGDLLIMPORT on everything, I'd agree
with you, but we clearly don't. Something's only a bug if we intended
A but accidentally got B. If we intended and got A and somebody
doesn't like that, that's not a bug; that's a difference of opinion.
I find it a stretch that there is intent involved here. Your comments below further that belief.
I personally feel that we should sprinkle PGDLLIMPORT markings onto a
lot more things, but Tom Lane has opposed that at every turn. I hope
we'll change our policy about that someday, but that's a different
question from whether such changes should be back-patched.
Different but related.
I'm sure the opinion I've expressed has supporters but their isn't enough bashing of us by the community at large that this is likely to get the decision makers to switch their feelings.
David J.
On Sat, Jun 4, 2016 at 2:56 AM, Robert Haas <robertmhaas@gmail.com> wrote: > On Wed, Jun 1, 2016 at 7:39 PM, Michael Paquier > <michael.paquier@gmail.com> wrote: >>> In short, I'd vote for putting this change in HEAD, but I see no need to >>> back-patch. >> >> OK, fine for me. > > Done. Thanks you. -- Michael