Re: Fix for FETCH FIRST syntax problems

[Vik Fearing]

On 20/05/18 01:41, Tom Lane wrote:
> Vik Fearing  writes:
>> On 20/05/18 00:57, Tom Lane wrote:
>> I'm +1 for backpatching it.  It may be operating as designed by PeterE
>> ten years ago, but it's not operating as designed by the SQL standard.
> 
> By that argument, *anyplace* where we're missing a SQL-spec feature
> is a back-patchable bug.  I don't buy it.

Only features we claim to support.  I obviously wouldn't consider
backpatching ASSERTIONs, for example.

> It may be that this fix is simple and safe enough that the risk/reward
> tradeoff favors back-patching, but I think you have to argue it as a
> favorable tradeoff rather than just saying "this isn't per standard".
> Consider: if Andrew had completely rewritten gram.y to get the same
> visible effect, would you think that was back-patchable?

Is the decision to backpatch based on behavior, or code churn?
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: Fix for FETCH FIRST syntax problems

[Vik Fearing]

On 20/05/18 05:25, Andrew Gierth wrote:
> +select_fetch_first_value:
> + c_expr  
> { $$ = $1; }
> + | '+' I_or_F_const
> + { $$ = (Node *) makeSimpleA_Expr(AEXPR_OP, "+", 
> NULL, $2, @1); }
> + | '-' I_or_F_const
> + { $$ = doNegate($2, @1); }

I think there should be a comment for why you're accepting FCONST when
the value has to be integral.
-- 
Vik Fearing  +33 6 46 75 15 36
http://2ndQuadrant.fr PostgreSQL : Expertise, Formation et Support

Re: Fix for FETCH FIRST syntax problems

[Peter Geoghegan]

On Sat, May 19, 2018 at 4:41 PM, Tom Lane  wrote:
> It may be that this fix is simple and safe enough that the risk/reward
> tradeoff favors back-patching, but I think you have to argue it as a
> favorable tradeoff rather than just saying "this isn't per standard".
> Consider: if Andrew had completely rewritten gram.y to get the same
> visible effect, would you think that was back-patchable?

I strongly agree with the general principle that back-patching a bug
fix needs to have a benefit that outweighs its cost. There have been
cases where we chose to not back-patch an unambiguous bug fix even
though it was clear that incorrect user-visible behavior remained.
Conversely, there have been cases where we back-patched a commit that
was originally introduced as a performance feature.

Whether or not Andrew's patch is formally classified as a bug fix is
subjective. I'm inclined to accept it as a bug fix, but I also think
that it shouldn't matter very much. The practical implication is that
I don't think it's completely out of the question to back-patch, but
AFAICT nobody else thinks it's out of the question anyway. Why bother
debating something that's inconsequential?

FWIW, I am neutral on the important question of whether or not this
patch should actually be back-patched.

-- 
Peter Geoghegan

Re: Fix for FETCH FIRST syntax problems

[David Fetter]

On Sun, May 20, 2018 at 01:39:27AM +0100, Andrew Gierth wrote:
> > "Tom" == Tom Lane  writes:
> 
>  >> I'm +1 for backpatching it. It may be operating as designed by
>  >> PeterE ten years ago, but it's not operating as designed by the SQL
>  >> standard.
> 
>  Tom> By that argument, *anyplace* where we're missing a SQL-spec
>  Tom> feature is a back-patchable bug. I don't buy it.
> 
> But this is a feature we already claimed to actually support (it's
> listed in sql_features with a bunch of unqualified YES entries), but in
> fact doesn't work properly.

This looks like a bug fix to me, for what it's worth.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Fix for FETCH FIRST syntax problems

[David G. Johnston]

On Sun, May 20, 2018 at 1:13 PM, Peter Geoghegan  wrote:

> There have been
> cases where we chose to not back-patch an unambiguous bug fix even
> though it was clear that incorrect user-visible behavior remained.
>

​The risk here is significantly reduced since the existing user-visible
behavior is an error which presumably no one is relying upon.  Between that
and being able to conform to the standard syntax for a long-standing
feature I would say the benefit outweighs the cost and risk.

+0.5 to back-patching

David J.
​

Re: Fix for FETCH FIRST syntax problems

[Tom Lane]

"David G. Johnston"  writes:
> ​The risk here is significantly reduced since the existing user-visible
> behavior is an error which presumably no one is relying upon.  Between that
> and being able to conform to the standard syntax for a long-standing
> feature I would say the benefit outweighs the cost and risk.

The risk you're ignoring is that this patch will break something that
*did* work before.  Given that the first version did exactly that,
I do not think that risk should be considered negligible.  I'm going
to change my vote for back-patching from -0.5 to -1.

regards, tom lane

Re: Fix for FETCH FIRST syntax problems

[Stephen Frost]

Greetings,

* Peter Geoghegan (p...@bowt.ie) wrote:
> Whether or not Andrew's patch is formally classified as a bug fix is
> subjective. I'm inclined to accept it as a bug fix, but I also think
> that it shouldn't matter very much. The practical implication is that
> I don't think it's completely out of the question to back-patch, but
> AFAICT nobody else thinks it's out of the question anyway. Why bother
> debating something that's inconsequential?

Just to be clear, based on what I saw on IRC, this specifically came out
of someone complaining that it didn't work and caused difficulty for
them.  As such, my inclination on this would be to back-patch it, but
we'd need to be sure to test it and be confident that it won't break
things which worked before.

Thanks!

Stephen


signature.asc
Description: PGP signature

RE: [HACKERS] Transactions involving multiple postgres foreign servers

[Tsunakawa, Takayuki]

From: Masahiko Sawada [mailto:sawada.m...@gmail.com]
> Regarding to API design, should we use 2PC for a distributed
> transaction if both two or more 2PC-capable foreign servers and
> 2PC-non-capable foreign server are involved with it?  Or should we end
> up with an error? the 2PC-non-capable server might be either that has
> 2PC functionality but just disables it or that doesn't have it.

>but I think we also could take
> the latter way because it doesn't make sense for user even if the
> transaction commit atomically among not all participants.

I'm for the latter.  That is, COMMIT or PREPARE TRANSACTION statement issued 
from an application reports an error.  DBMS, particularly relational DBMS (, 
and even more particularly Postgres?) places high value on data correctness.  
So I think transaction atomicity should be preserved, at least by default.  If 
we preferred updatability and performance to data correctness, why don't we 
change the default value of synchronous_commit to off in favor of performance?  
On the other hand, if we want to allow 1PC commit when not all FDWs support 
2PC, we can add a new GUC parameter like "allow_nonatomic_commit = on", just 
like synchronous_commit and fsync trade-offs data correctness and performance.


> Also, regardless whether we take either way  I think it would be
> better to manage not only 2PC transaction but also non-2PC transaction
> in the core and add two_phase_commit argument. I think we can use it
> without breaking existing FDWs. Currently FDWs manage transactions
> using XactCallback but new APIs being added also manage transactions.
> I think it might be better if users use either way (using XactCallback
> or using new APIs) for transaction management rather than use both
> ways with combination. Otherwise two codes for transaction management
> will be required: the code that manages foreign transactions using
> XactCallback for non-2PC transactions and code that manages them using
> new APIs for 2PC transactions. That would not be easy for FDW
> developers. So what I imagined for new API is that if FDW developers
> use new APIs they can use both 2PC and non-2PC transaction, but if
> they use XactCallback they can use only non-2PC transaction.
> Any thoughts?

If we add new functions, can't we just add functions whose names are 
straightforward like PrepareTransaction() and CommitTransaction()?  FDWs 
without 2PC support returns NULL for the function pointer of 
PrepareTransaction().

This is similar to XA: XA requires each RM to provide function pointers for 
xa_prepare() and xa_commit().  If we go this way, maybe we could leverage the 
artifact of postgres_fdw to create the XA library for C/C++.  I mean we put 
transaction control functions in the XA library, and postgres_fdw also uses it. 
 i.e.:

postgres_fdw.so -> libxa.so -> libpq.so
 \-/


Regards
Takayuki Tsunakawa

Re: [HACKERS] Replication status in logical replication

[Masahiko Sawada]

On Tue, Apr 10, 2018 at 11:11 PM, David Steele  wrote:
> On 4/10/18 6:14 AM, Masahiko Sawada wrote:
>> On Fri, Mar 30, 2018 at 5:37 AM, Fujii Masao  wrote:
>>> On Wed, Jan 17, 2018 at 2:16 AM, Simon Riggs  wrote:
 On 16 January 2018 at 06:21, Michael Paquier  
 wrote:
> On Tue, Jan 16, 2018 at 10:40:43AM +0900, Masahiko Sawada wrote:
>> On Sun, Jan 14, 2018 at 12:43 AM, Simon Riggs  
>> wrote:
>>> On 9 January 2018 at 04:36, Masahiko Sawada  
>>> wrote:
>>> We're not talking about standbys, so the message is incorrect.
>>
>> Ah, I understood. How about "\"%s\"  has now caught up with upstream
>> server"?
>
> +1.

 upstream is what I would have suggested, so +1 here also.

 Will commit.
>>>
>>> ping?
>>>
>>
>> Should I add this item to "Older Bugs" of the open item since we
>> regarded it as a bug?
>
> I'm going to reclassify this as a bug since everyone seems to agree it
> is one.

I've added this to Open Items so as not to forget.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Add necessary package list to ldap TAP's README

[Michael Paquier]

Hi all,

The kerberos test suite mentions the package list to use on a set of
distributions, which is very useful.  However we don't do the same for
ldap.  Something like the attached would be adapted then to help setting
up a test environment?

I have at hand only a Debian installation, and I have just guessed the
RHEL and FreeBSD dependencies.

Thanks,
--
Michael
diff --git a/src/test/ldap/README b/src/test/ldap/README
index 61578385c5..3e4c5cbaf9 100644
--- a/src/test/ldap/README
+++ b/src/test/ldap/README
@@ -22,3 +22,14 @@ Running the tests
 or
 
 make installcheck
+
+Requirements
+
+
+LDAP server and client tools are required.
+
+Debian/Ubuntu packages: slapd ldap-utils
+
+RHEL/CentOS packages: openldap openldap-clients openldap-servers
+
+FreeBSD: openldap24-server openldap24-client


signature.asc
Description: PGP signature

Allowing printf("%m") only where it actually works

[Tom Lane]

For amusement's sake, I was playing around with NetBSD-current (9-to-be)
today, and tried to compile Postgres on it.  It works OK --- and I can
even confirm that our new code for using ARM v8 CRC instructions works
there --- but I got a boatload of compile warnings like this:

latch.c:1180:4: warning: %m is only allowed in syslog(3) like functions 
[-Wformat=]
ereport(ERROR,
^~~

A bit of googling turned up the patch that caused this [1], which was
soon followed by some well-reasoned push-back [2]; but the warning's
still there, so evidently the forces of bullheadedness won.  I was
ready to discount the whole thing as being another badly designed
no-wonder-gcc-upstream-won't-take-it compiler warning, when I noticed that
the last few warnings in my output were pointing out a live bug, to wit
using %m with plain old printf rather than elog/ereport.  So I fixed
that [3], but I'm thinking that we need to take a bit more care here.



Looking around, we have circa seventy-five functions declared with
pg_attribute_printf in our tree right now, and of those, *only* the
elog/ereport support functions can be relied on to process %m correctly.
However, anyone who's accustomed to working in backend code is likely to
not think hard about using %m in an error message, as indeed the authors
and reviewers of pg_verify_checksums did not.  Worse yet, such cases
actually will work as long as you're testing on glibc platforms, only
to fail everywhere else.

So I think we need to try to make provisions for getting compiler warnings
when %m is used in a function that doesn't support it.  gcc on Linux-ish
platforms isn't going to be very helpful with this, but that doesn't mean
that we should confuse %m-supporting and not-%m-supporting functions,
as we do right now.

Hence, I think we need something roughly like the attached patch, which
arranges to use "gnu_printf" (if available) as the format archetype for
the elog/ereport functions, and plain "printf" for all the rest.  With
some additional hackery not included here, this can be ju-jitsu'd into
compiling warning-free on NetBSD-current.  (The basic idea is to extend
PGAC_C_PRINTF_ARCHETYPE so it will select __syslog__ as the archetype
if available; but then you need some hack to suppress the follow-on
warnings complained of in [2].  I haven't decided what's the least ugly
solution for the latter, so I'm not proposing such a patch yet.)

What I'm mainly concerned about at this stage is what effects this'll
have on Windows builds.  The existing comment for PGAC_C_PRINTF_ARCHETYPE
claims that using gnu_printf silences complaints about "%lld" and related
formats on Windows, but I wonder whether that is still true on Windows
versions we still support.  As I mentioned in [4], I don't think we really
work any longer on platforms that don't use "%lld" for "long long" values,
and it seems that Windows does accept that in post-XP versions --- but has
gcc gotten the word?

If this does work as desired on Windows, then that would be a fairly
mainstream platform that can produce warnings about wrong uses of %m,
even if gcc-on-Linux doesn't.  If worst comes to worst, somebody could
crank up a buildfarm machine with a newer NetBSD release.

Anyway, I don't feel a need to cram this into v11, since I just fixed
the live bugs of this ilk in HEAD; it seems like it can wait for v12.
So I'll add this to the next commitfest.

regards, tom lane

[1] https://mail-index.netbsd.org/tech-userlevel/2015/08/21/msg009282.html
[2] https://mail-index.netbsd.org/tech-userlevel/2015/10/23/msg009371.html
[3] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=a13b47a59ffce6f3c13c8b38a3aab1db10d3
[4] https://www.postgresql.org/message-id/13103.1526749...@sss.pgh.pa.us

diff --git a/config/c-compiler.m4 b/config/c-compiler.m4
index ba5c40d..5cd8994 100644
--- a/config/c-compiler.m4
+++ b/config/c-compiler.m4
@@ -19,10 +19,10 @@ fi])# PGAC_C_SIGNED
 
 # PGAC_C_PRINTF_ARCHETYPE
 # ---
-# Set the format archetype used by gcc to check printf type functions.  We
-# prefer "gnu_printf", which includes what glibc uses, such as %m for error
-# strings and %lld for 64 bit long longs.  GCC 4.4 introduced it.  It makes a
-# dramatic difference on Windows.
+# Set the format archetype used by gcc to check elog/ereport functions.
+# This should accept %m, whether or not the platform's printf does.
+# We use "gnu_printf" if possible, which does that, although in some cases
+# it might do more than we could wish.
 AC_DEFUN([PGAC_PRINTF_ARCHETYPE],
 [AC_CACHE_CHECK([for printf format archetype], pgac_cv_printf_archetype,
 [ac_save_c_werror_flag=$ac_c_werror_flag
@@ -34,8 +34,8 @@ __attribute__((format(gnu_printf, 2, 3)));], [])],
   [pgac_cv_printf_archetype=gnu_printf],
   [pgac_cv_printf_archetype=printf])
 ac_c_werror_flag=$ac_save_c_werror_flag])
-AC_DEFINE_UNQUOTED([PG_PRINTF_ATTRIBUTE], 

Re: Fix for FETCH FIRST syntax problems

[Andrew Gierth]

> "Stephen" == Stephen Frost  writes:

 Stephen> Just to be clear, based on what I saw on IRC, this
 Stephen> specifically came out of someone complaining that it didn't
 Stephen> work and caused difficulty for them.

Specifically, as I said at the start, it's from bug #15200, see
http://postgr.es/m/152647780335.27204.16895288237122418...@wrigleys.postgresql.org

 Stephen> As such, my inclination on this would be to back-patch it, but
 Stephen> we'd need to be sure to test it and be confident that it won't
 Stephen> break things which worked before.

Well, that's kind of what I have been doing (and why I posted a second
version of the patch).

So let's go over the full detail. The old syntax is:

   OFFSET select_offset_value2 row_or_rows
   FETCH first_or_next opt_select_fetch_first_value row_or_rows ONLY

   opt_select_fetch_first_value: SignedIconst | '(' a_expr ')' | /*EMPTY*/;
   select_offset_value2: c_expr;
   SignedIconst: Iconst | '+' Iconst | '-' Iconst;

The new syntax is:

   OFFSET select_fetch_first_value row_or_rows
   FETCH first_or_next select_fetch_first_value row_or_rows ONLY
   FETCH first_or_next row_or_rows ONLY

   select_fetch_first_value: c_expr | '+' I_or_F_const | '-' I_or_F_const;

In both cases note that the sequence '(' a_expr ')' is a valid c_expr.

The old syntax for OFFSET x ROWS clearly simplifies to

  OFFSET c_expr [ROW|ROWS]

and the new syntax clearly accepts a strict superset of that, since it
just adds +/- I_or_F_const as an alternative to c_expr, fixing the
(probably inconsequential) bug that OFFSET +1 ROWS didn't work despite
being legal in the spec.

The old syntax for FETCH FIRST expands to:

 1)   FETCH first_or_next Iconst row_or_rows ONLY
 2)   FETCH first_or_next '+' Iconst row_or_rows ONLY
 3)   FETCH first_or_next '-' Iconst row_or_rows ONLY
 4)   FETCH first_or_next '(' a_expr ')' row_or_rows ONLY
 5)   FETCH first_or_next row_or_rows ONLY

5) is explicitly matched in the new syntax. 1) and 4) both match via the
fact that Iconst and '(' a_expr ')' are valid for c_expr. 2) and 3) are
matched in the new syntax with the addition of accepting FCONST as well
as Iconst.

So all input that satisfied the old syntax will be accepted by the new
one.

Of course, I have done actual tests comparing the old and new versions,
with results consistent with the above. I do note that there seems to be
no test coverage at all - none was added in commit 361bfc357 which
created the feature, and nobody seems to have cared about it since other
than some doc tweaks.

I've also checked (by splitting into separate ROW and ROWS alternatives)
that there aren't any grammar conflicts being "hidden" inappropriately
by precedence (ROWS has a precedence assigned, but ROW does not).
Inspection of the bison verbose output confirms this.

Normally when messing with the grammar one would also have to consider
the effect on dump/restore. But in this case, we never actually generate
this syntax when deparsing - offset/limit clauses are always deparsed as
the OFFSET x LIMIT y syntax instead. So we don't have to worry about,
for example, dumping from a newer point release to an older one; the
only time we see this syntax is if the client generates it.

-- 
Andrew (irc:RhodiumToad)

Re: Odd procedure resolution

[Ashutosh Bapat]

On Sat, May 19, 2018 at 12:51 AM, Robert Haas  wrote:
> On Thu, May 17, 2018 at 4:10 PM, Peter Eisentraut
>  wrote:
>> I think I have made a mistake here.  I was reading in between the lines
>> of a competitor's documentation that they have functions and procedures
>> in different name spaces, which made me re-read the SQL standard, which
>> appears to support that approach.
>
> I am really doubtful about trying to merge those completely.  You end
> up with confusion about what DROP ROUTINE actually means, for example.
> Also, I am quite dubious about the idea that functions, window
> functions, and aggregates should go all together into one namespace
> and procedures into a completely different one.  I thought merging all
> of that stuff down into prokind was quite elegant, and I'm not too
> excited about seeing that change backed out.  Functions, procedures,
> aggregates, and window functions are all function-like things -- given
> any one of them, you might end up writing something like
> mything(thingarg1, thingarg2) in some context or other.  I think it is
> very sensible to say that we won't let you create two such things with
> identical signature, because that's just confusing -- and probably of
> very doubtful utility.  At the same time, I don't think that precludes
> using context clues to figure out which one must have been intended in
> a particular SQL statement.  There are cases where something must
> "become all one thing or all the other", but I don't see why that
> should be true here.

+1 for all that.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Allowing printf("%m") only where it actually works

[Thomas Munro]

On Mon, May 21, 2018 at 12:30 PM, Tom Lane  wrote:
> For amusement's sake, I was playing around with NetBSD-current (9-to-be)
> today, and tried to compile Postgres on it.  It works OK --- and I can
> even confirm that our new code for using ARM v8 CRC instructions works

Excellent news.

> there --- but I got a boatload of compile warnings like this:
>
> latch.c:1180:4: warning: %m is only allowed in syslog(3) like functions 
> [-Wformat=]
> ereport(ERROR,
> ^~~
>
> A bit of googling turned up the patch that caused this [1], which was
> soon followed by some well-reasoned push-back [2]; but the warning's
> still there, so evidently the forces of bullheadedness won.  I was
> ready to discount the whole thing as being another badly designed
> no-wonder-gcc-upstream-won't-take-it compiler warning, when I noticed that
> the last few warnings in my output were pointing out a live bug, to wit
> using %m with plain old printf rather than elog/ereport.  So I fixed
> that [3], but I'm thinking that we need to take a bit more care here.

I tried this on macOS and FreeBSD using GCC and Clang: both accept
printf("%m") without warning and then just print out "m".  It'll be
interesting to see if the NetBSD patch/idea travels further or some
other solution can be found.  I've raised this on the freebsd-hackers
list, let's see... I bet there's other software out there that just
prints out "m" when things go wrong.  It's arguably something that
you'd want the complier to understand as a C dialect thing.

-- 
Thomas Munro
http://www.enterprisedb.com

Re: Postgres, fsync, and OSs (specifically linux)

[Ashutosh Bapat]

On Sat, May 19, 2018 at 6:31 AM, Stephen Frost  wrote:
> Greetings,
>
> * Abhijit Menon-Sen (a...@2ndquadrant.com) wrote:
>> At 2018-05-18 20:27:57 -0400, sfr...@snowman.net wrote:
>> >
>> > I don't agree with the general notion that we can't have a function
>> > which handles the complicated bits about the kind of error because
>> > someone grep'ing the source for PANIC might have to do an additional
>> > lookup.
>>
>> Or we could just name the function promote_eio_to_PANIC.
>
> Ugh, I'm not thrilled with that either.
>
>> (I understood the objection to be about how 'grep PANIC' wouldn't find
>> these lines at all, not that there would be an additional lookup.)
>
> ... and my point was that 'grep PANIC' would, almost certainly, find the
> function promote_eio_to_panic(), and someone could trivially look up all
> the callers of that function then.

It's not just grep,  but tools like cscope, tag. Although, I agree,
that adding a function, if all necessary, is more important than
convenience of finding all the instances of a certain token easily.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

Thomas Munro  writes:
> I tried this on macOS and FreeBSD using GCC and Clang: both accept
> printf("%m") without warning and then just print out "m".  It'll be
> interesting to see if the NetBSD patch/idea travels further or some
> other solution can be found.  I've raised this on the freebsd-hackers
> list, let's see... I bet there's other software out there that just
> prints out "m" when things go wrong.  It's arguably something that
> you'd want the complier to understand as a C dialect thing.

Yeah.  ISTM that the netbsd guys didn't get this quite right.  The
gcc docs are perfectly clear about what they think the semantics should
be:

The parameter archetype determines how the format string is
interpreted, and should be printf, scanf, strftime, gnu_printf,
gnu_scanf, gnu_strftime or strfmon. ...  archetype values such as
printf refer to the formats accepted by the system’s C runtime
library, while values prefixed with ‘gnu_’ always refer to the formats
accepted by the GNU C Library.

Therefore, what netbsd should have done was leave the semantics of
"gnu_printf" alone (because glibc undoubtedly does take %m), and just emit
a warning with the "printf" archetype --- which is supposed to describe
the behavior of the platform's stdio, which in their case is known not
to take %m.  If they'd done it that way then they'd have a patch that gcc
upstream certainly ought to accept, because it follows gcc's own stated
semantics for the check.  This would also partially resolve the complaint
Roy Marples had in the thread I alluded to, ie he could use "gnu_printf"
to describe a function that accepts %m.  (There might still need to be
some work to be done to avoid bogus -Wmissing-format-attribute complaints,
not sure.)  I'm not sure whether a separate archetype for syslog is really
needed.  Formally you could say that distinguishing syslog from GNU printf
is wise, but I'm not sure I see a practical need for it.

regards, tom lane

Re: Allowing printf("%m") only where it actually works

[Tom Lane]

... and, while we're thinking about this, how can we prevent the reverse
problem of using strerror(errno) where you should have used %m?  That
is evidently not academic either, cf 81256cd.

I am wondering whether the elog/ereport macros can locally define some
version of "errno" that would cause a compile failure if it's referenced
within the macro args.  But I'm too tired to work it out in detail.

regards, tom lane

Re: Postgres, fsync, and OSs (specifically linux)

[Craig Ringer]

On 18 May 2018 at 00:44, Andres Freund  wrote:

> Hi,
>
> On 2018-05-10 09:50:03 +0800, Craig Ringer wrote:
> >   while ((src = (RewriteMappingFile *) hash_seq_search(_status))
> != NULL)
> >   {
> >   if (FileSync(src->vfd, WAIT_EVENT_LOGICAL_REWRITE_SYNC)
> != 0)
> > - ereport(ERROR,
> > + ereport(PANIC,
> >   (errcode_for_file_access(),
> >errmsg("could not fsync file
> \"%s\": %m", src->path)));
>
> To me this (and the other callers) doesn't quite look right. First, I
> think we should probably be a bit more restrictive about when PANIC
> out. It seems like we should PANIC on ENOSPC and EIO, but possibly not
> others.  Secondly, I think we should centralize the error handling. It
> seems likely that we'll acrue some platform specific workarounds, and I
> don't want to copy that knowledge everywhere.
>
> Also, don't we need the same on close()?
>
>
Yes, we do, and that expands the scope a bit.

I agree with Robert that some sort of filter/macro is wise, though naming
it clearly will be tricky.

I'll have a look.

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