Re: amcheck verification for GiST

[Andrey Borodin]

Hi!

> 24 сент. 2018 г., в 8:29, Thomas Munro  
> написал(а):
> 
> On Sun, Sep 23, 2018 at 10:15 PM Andrey Borodin  wrote:
>> Here's the patch with amcheck functionality for GiST.
> 
> Hi Andrey,
> 
> Windows doesn't like it[1]:

Thanks, Thomas! Yes, I've missed that version-dependent macro. Surely, it's 
redundant.

Best regards, Andrey Borodin.


0001-GiST-verification-function-for-amcheck-v2.patch
Description: Binary data

Re: Pluggable Storage - Andres's take

[Haribabu Kommi]

On Mon, Sep 24, 2018 at 5:02 AM Alexander Korotkov <
a.korot...@postgrespro.ru> wrote:

> On Fri, Aug 24, 2018 at 5:50 AM Andres Freund  wrote:
> > I've pushed a current version of that to my git tree to the
> > pluggable-storage branch. It's not really a version that I think makese
> > sense to review or such, but it's probably more useful if you work based
> > on that.  There's also the pluggable-zheap branch, which I found
> > extremely useful to develop against.
>
> BTW, I'm going to take a look at current shape of this patch and share
> my thoughts.  But where are the branches you're referring?  On your
> postgres.org git repository pluggable-storage brach was updates last
> time at June 7.  And on the github branches are updated at August 5
> and 14, and that is still much older than your email (August 24)...
>

The code is latest, but the commit time is older, I feel that is because of
commit squash.

pluggable-storage is the branch where the pluggable storage code is present
and pluggable-zheap branch where zheap is rebased on top of pluggable
storage.

Regards,
Haribabu Kommi
Fujitsu Australia

Re: amcheck verification for GiST

[Andres Freund]

On 2018-09-24 15:29:38 +1200, Thomas Munro wrote:
> On Sun, Sep 23, 2018 at 10:15 PM Andrey Borodin  wrote:
> > Here's the patch with amcheck functionality for GiST.
> 
> Hi Andrey,
> 
> Windows doesn't like it[1]:
> 
> contrib/amcheck/verify_gist.c(163): error C2121: '#' : invalid
> character : possibly the result of a macro expansion
> [C:\projects\postgresql\amcheck.vcxproj]
> 
> That's:
> 
> +MemoryContext mctx = AllocSetContextCreate(CurrentMemoryContext,
> + "amcheck context",
> +#if PG_VERSION_NUM >= 11
> + ALLOCSET_DEFAULT_SIZES);
> +#else
> + ALLOCSET_DEFAULT_MINSIZE,
> + ALLOCSET_DEFAULT_INITSIZE,
> + ALLOCSET_DEFAULT_MAXSIZE);
> +#endif
> 
> Not sure what's gong on there... perhaps it doesn't like you to do
> that in the middle of a function-like-macro invocation
> (AllocSetContextCreate)?

But note that the version dependent code shouldn't be present in
/contrib anyway.

Greetings,

Andres Freund

Re: amcheck verification for GiST

[Thomas Munro]

On Sun, Sep 23, 2018 at 10:15 PM Andrey Borodin  wrote:
> Here's the patch with amcheck functionality for GiST.

Hi Andrey,

Windows doesn't like it[1]:

contrib/amcheck/verify_gist.c(163): error C2121: '#' : invalid
character : possibly the result of a macro expansion
[C:\projects\postgresql\amcheck.vcxproj]

That's:

+MemoryContext mctx = AllocSetContextCreate(CurrentMemoryContext,
+ "amcheck context",
+#if PG_VERSION_NUM >= 11
+ ALLOCSET_DEFAULT_SIZES);
+#else
+ ALLOCSET_DEFAULT_MINSIZE,
+ ALLOCSET_DEFAULT_INITSIZE,
+ ALLOCSET_DEFAULT_MAXSIZE);
+#endif

Not sure what's gong on there... perhaps it doesn't like you to do
that in the middle of a function-like-macro invocation
(AllocSetContextCreate)?

[1] https://ci.appveyor.com/project/postgresql-cfbot/postgresql/build/1.0.14056

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

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Haribabu Kommi]

On Mon, Sep 24, 2018 at 11:13 AM Haribabu Kommi 
wrote:

> On Sat, Sep 22, 2018 at 11:23 PM Amit Kapila 
> wrote:
>
>> On Mon, Jul 9, 2018 at 11:28 AM Haribabu Kommi 
>> wrote:
>> >
>> > On Mon, Jul 9, 2018 at 3:39 PM Haribabu Kommi 
>> wrote:
>> >>
>> >>
>> >> On Mon, Jul 9, 2018 at 12:24 PM Michael Paquier 
>> wrote:
>> >>>
>> >>> On Fri, Jul 06, 2018 at 05:10:18PM -0400, Alvaro Herrera wrote:
>> >>> > Ugh, it's true :-(
>> >>> >
>> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8
>> >>> >
>> >>> > Dave, Simon, any comments?
>> >>>
>> >>> The offending line:
>> >>> contrib/pg_stat_statements/pg_stat_statements--1.4--1.5.sql:
>> >>> GRANT EXECUTE ON FUNCTION pg_stat_statements_reset() TO
>> pg_read_all_stats;
>> >>>
>> >>> This will need a new version bump down to REL_10_STABLE...
>> >>
>> >>
>> >> Hearing no objections, attached patch removes all permissions from
>> PUBLIC
>> >> as before this change went in. Or do we need to add command for revoke
>> only
>> >> from pg_read_all_stats?
>> >
>> >
>> > Revoke all doesn't work, so patch updated with revoke from
>> pg_read_all_stats.
>> >
>>
>
> Thanks for the review.
>
>
>> The other questionable part of that commit (25fff40798) is that it
>> changes permissions for function pg_stat_statements_reset at SQL level
>> and for C function it changes the permission check for
>> pg_stat_statements, refer below change.
>>
>
> The below changes are to support the read access of particular columns
> of the pg_stat_statements view to pg_read_all_stats role. These changes
> should exist and only the permissions of pg_stat_statements_reset()
> function
> needs to be removed.
>
>
>> @@ -1391,7 +1392,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>> MemoryContext per_query_ctx;
>> MemoryContext oldcontext;
>> Oid userid = GetUserId();
>> -   boolis_superuser = superuser();
>> +   boolis_allowed_role = false;
>> char   *qbuffer = NULL;
>> Sizeqbuffer_size = 0;
>> Sizeextent = 0;
>> @@ -1399,6 +1400,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
>> HASH_SEQ_STATUS hash_seq;
>> pgssEntry  *entry;
>>
>> +   /* Superusers or members of pg_read_all_stats members are allowed */
>> +   is_allowed_role = is_member_of_role(GetUserId(),
>> DEFAULT_ROLE_READ_ALL_STATS);
>>
>> Am I confused here?  In any case, I think it is better to start a
>> separate thread to discuss this issue.  It might help us in getting
>> more attention on this issue and we can focus on your proposed patch
>> in this thread.
>>
>
> Started a new thread to discuss about the revoke the execute permissions
> of the pg_stat_statements_reset() from pg_read_all_stats role at [1]
>

Attached new rebased version of the patch that enhances the
pg_stat_statements_reset()
function. This needs to be applied on top of the patch that is posted in
[1].

[1] -
https://www.postgresql.org/message-id/CAJrrPGf5fCnKqXObpwGN9nMyD--tzOf-7LFCJiz59Z1wJ5qj9A%40mail.gmail.com

Regards,
Haribabu Kommi
Fujitsu Australia


0001-pg_stat_statements_reset-to-reset-specific-query-use_v5.patch
Description: Binary data

Re: New function pg_stat_statements_reset_query() to reset statistics of a specific query

[Haribabu Kommi]

On Sat, Sep 22, 2018 at 11:23 PM Amit Kapila 
wrote:

> On Mon, Jul 9, 2018 at 11:28 AM Haribabu Kommi 
> wrote:
> >
> > On Mon, Jul 9, 2018 at 3:39 PM Haribabu Kommi 
> wrote:
> >>
> >>
> >> On Mon, Jul 9, 2018 at 12:24 PM Michael Paquier 
> wrote:
> >>>
> >>> On Fri, Jul 06, 2018 at 05:10:18PM -0400, Alvaro Herrera wrote:
> >>> > Ugh, it's true :-(
> >>> >
> https://git.postgresql.org/gitweb/?p=postgresql.git;a=commitdiff;h=25fff40798fc4ac11a241bfd9ab0c45c085e2212#patch8
> >>> >
> >>> > Dave, Simon, any comments?
> >>>
> >>> The offending line:
> >>> contrib/pg_stat_statements/pg_stat_statements--1.4--1.5.sql:
> >>> GRANT EXECUTE ON FUNCTION pg_stat_statements_reset() TO
> pg_read_all_stats;
> >>>
> >>> This will need a new version bump down to REL_10_STABLE...
> >>
> >>
> >> Hearing no objections, attached patch removes all permissions from
> PUBLIC
> >> as before this change went in. Or do we need to add command for revoke
> only
> >> from pg_read_all_stats?
> >
> >
> > Revoke all doesn't work, so patch updated with revoke from
> pg_read_all_stats.
> >
>

Thanks for the review.


> The other questionable part of that commit (25fff40798) is that it
> changes permissions for function pg_stat_statements_reset at SQL level
> and for C function it changes the permission check for
> pg_stat_statements, refer below change.
>

The below changes are to support the read access of particular columns
of the pg_stat_statements view to pg_read_all_stats role. These changes
should exist and only the permissions of pg_stat_statements_reset() function
needs to be removed.


> @@ -1391,7 +1392,7 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> MemoryContext per_query_ctx;
> MemoryContext oldcontext;
> Oid userid = GetUserId();
> -   boolis_superuser = superuser();
> +   boolis_allowed_role = false;
> char   *qbuffer = NULL;
> Sizeqbuffer_size = 0;
> Sizeextent = 0;
> @@ -1399,6 +1400,9 @@ pg_stat_statements_internal(FunctionCallInfo fcinfo,
> HASH_SEQ_STATUS hash_seq;
> pgssEntry  *entry;
>
> +   /* Superusers or members of pg_read_all_stats members are allowed */
> +   is_allowed_role = is_member_of_role(GetUserId(),
> DEFAULT_ROLE_READ_ALL_STATS);
>
> Am I confused here?  In any case, I think it is better to start a
> separate thread to discuss this issue.  It might help us in getting
> more attention on this issue and we can focus on your proposed patch
> in this thread.
>

Started a new thread to discuss about the revoke the execute permissions
of the pg_stat_statements_reset() from pg_read_all_stats role at [1]

[1] -
https://www.postgresql.org/message-id/CAJrrPGf5fCnKqXObpwGN9nMyD--tzOf-7LFCJiz59Z1wJ5qj9A%40mail.gmail.com

Regards,
Haribabu Kommi
Fujitsu Australia

Re: Missing const in DSA.

[Thomas Munro]

On Mon, Sep 24, 2018 at 9:32 AM Tom Lane  wrote:
> Mark G  writes:
> > While looking at some of the recent churn in DSA I noticed that
> > dsa_size_class_map should probably be declared const.
>
> +1 ... also, given the contents of the array, "char" seems like
> rather a misnomer.  I'd be happier if it were declared as uint8, say.

+1

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

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 >> select nth_value(x) from first ignore;

 Tom> No, because once IGNORE is a keyword, even unreserved, it's not
 Tom> legal as an AS-less alias.

That rule only applies in the select-list, not in the FROM clause; table
aliases in FROM are just ColId, so they can be anything except a fully
reserved or type_func_name keyword.

-- 
Andrew (irc:RhodiumToad)

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> If you just think of recognizing FROM FIRST/LAST, you get nowhere
>  Tom> because that's still legal in other contexts. But if you were to
>  Tom> look for FROM followed by FIRST/LAST followed by
>  Tom> IGNORE/RESPECT/OVER, I think that could only validly happen in
>  Tom> this syntax.

> No; you need to go four tokens ahead in total, not three. Assuming
> nth_value is unreserved, then
>   select nth_value(x) from first ignore;
> is a valid query that has nth_value(x) as an expression, "first" as a
> table name and "ignore" as its alias.

No, because once IGNORE is a keyword, even unreserved, it's not legal
as an AS-less alias.  We'd be breaking queries like that no matter what.

(I know there are people around here who'd like to remove that
restriction, but it's not happening anytime soon IMO.)

regards, tom lane

Re: Proposal for Signal Detection Refactoring

[Michael Paquier]

On Fri, Sep 21, 2018 at 12:35:46PM +0200, Chris Travers wrote:
> I understand how lock levels don't fit a simple hierarchy but at least
> when it comes to what is going to be aborted on a signal, I am having
> trouble understanding the problem here.

It may be possible to come with a clear hierarchy with the current
interruption types in place.  Still I am not sure that the definition
you put behind is completely correct, and I think that we need to
question as well the value of putting such restrictions for future
interruption types because they would need to fit into it.  That's quite
a heavy constraint to live with.  There is such logic with wal_level for
example, which is something I am not completely happy with either...
But this one is a story for another time, and another thread.

Regarding your patch, it seems to me that it does not improve
readability as I mentioned up-thread because you lose sight of what can
be interrupted in a given code path, which is what the current code
shows actually nicely.

There could be value in refactoring things so as all the *Pending flags
of miscadmin.h get stored into one single volatile sig_atomic_t which
uses bit-wise markers, as that's at least 4 bytes because that's stored
as an int for most platforms and can be performed as an atomic operation
safely across signals (If my memory is right;) ).  And this leaves a lot
of room for future flags.
--
Michael


signature.asc
Description: PGP signature

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> If you just think of recognizing FROM FIRST/LAST, you get nowhere
 Tom> because that's still legal in other contexts. But if you were to
 Tom> look for FROM followed by FIRST/LAST followed by
 Tom> IGNORE/RESPECT/OVER, I think that could only validly happen in
 Tom> this syntax.

No; you need to go four tokens ahead in total, not three. Assuming
nth_value is unreserved, then

  select nth_value(x) from first ignore;

is a valid query that has nth_value(x) as an expression, "first" as a
table name and "ignore" as its alias. Only when you see NULLS after
IGNORE, or OVER after FIRST/LAST, do you know that you're looking at
a window function and not a from clause.

So FROM_LA would have to mean "FROM" followed by any of:

  FIRST IGNORE NULLS
  LAST IGNORE NULLS
  FIRST RESPECT NULLS
  LAST RESPECT NULLS
  FIRST OVER
  LAST OVER

Remember that while OVER is reserved, all of FIRST, LAST, RESPECT and
IGNORE are unreserved.

 Tom> It'd take some work to extend base_yylex to look ahead 2 tokens
 Tom> not one, but I'm sure that could be done. (You'd also need a
 Tom> lookahead rule to match "IGNORE/RESPECT NULLS OVER", but that
 Tom> seems just as doable.) Then the relevant productions use FROM_LA,
 Tom> IGNORE_LA, RESPECT_LA instead of the corresponding bare tokens,
 Tom> and the grammar no longer has an ambiguity problem.

Yeah, but at the cost of having to extend base_yylex to go 3 tokens
ahead (not 2) rather than the current single lookahead slot.

Doable, certainly (probably not much harder to do 3 than 2 actually)

-- 
Andrew (irc:RhodiumToad)

Re: when set track_commit_timestamp on, database system abort startup

[Michael Paquier]

On Thu, Sep 20, 2018 at 09:51:34PM +0900, Masahiko Sawada wrote:
> I agreed with your all review comments. Attached the updated patch.

Wouldn't it be better to incorporate the new test as part of
004_restart.pl?  This way, we avoid initializing a full instance, which
is always a good thing as that's very costly.  The top of this file also
mentions that it tests clean restarts, but it bothers also about crash
recovery.

+$node->teardown_node;
I would suggest using $node->stop('immediate') here, which makes the
test easier to understand.
--
Michael


signature.asc
Description: PGP signature

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Tom Lane]

Andrew Gierth  writes:
> "Tom" == Tom Lane  writes:
>  Tom> The FROM FIRST/LAST bit seems particularly badly thought through,
>  Tom> because AFAICS it is flat out ambiguous with a normal FROM clause
>  Tom> immediately following the window function call. The only way to
>  Tom> make it not so would be to make FIRST and LAST be fully reserved,
>  Tom> which is neither a good idea nor spec-compliant.

> In the actual spec syntax it's not ambiguous at all because NTH_VALUE is
> a reserved word (as are LEAD, LAG, FIRST_VALUE and LAST_VALUE), and OVER
> is a mandatory clause in its syntax, so a FROM appearing before the OVER
> must be part of a FROM FIRST/LAST and not introducing a FROM-clause.

Hmm ...

> In our syntax, if we made NTH_VALUE etc. a col_name_keyword (and thus
> not legal as a function name outside its own special syntax) it would
> also become unambiguous.
> i.e. given this token sequence (with . marking the current posision):
>   select nth_value(x) . from first ignore 
> if we know up front that "nth_value" is a window function and not any
> other kind of function, we know that we have to shift the "from" rather
> than reducing the select-list because we haven't seen an "over" yet.

I don't really find that to be a desirable solution, because quite aside
from the extensibility problem, it would mean that a lot of errors
become "syntax error" where we formerly gave a more useful message.

This does open up a thought about how to proceed, though.  I'd been
trying to think of a way to solve this using base_yylex's ability to
do some internal lookahead and change token types based on that.
If you just think of recognizing FROM FIRST/LAST, you get nowhere
because that's still legal in other contexts.  But if you were to
look for FROM followed by FIRST/LAST followed by IGNORE/RESPECT/OVER,
I think that could only validly happen in this syntax.  It'd take
some work to extend base_yylex to look ahead 2 tokens not one, but
I'm sure that could be done.  (You'd also need a lookahead rule to
match "IGNORE/RESPECT NULLS OVER", but that seems just as doable.)
Then the relevant productions use FROM_LA, IGNORE_LA, RESPECT_LA
instead of the corresponding bare tokens, and the grammar no longer
has an ambiguity problem.

regards, tom lane

Re: Changing the setting of wal_sender_timeout per standby

[Michael Paquier]

On Sun, Sep 23, 2018 at 10:47:44AM -0400, Tom Lane wrote:
> Andres Freund  writes:
>> Have there been discussions about the security effects of this change?
>> Previously the server admin could control the timeout, which could
>> affect things like syncrep, after this it's not possible anymore.  I
>> *think* that's ok, but it should be discussed.
> 
> Hm.  An evil replication connection could already cause all sorts of
> operational problems (and I'm not counting grabbing all your data).
> Does this add anything much new in that line?  It seems like the
> effects would be at least in the same ballpark as not sending
> hot-standby-feedback messages in a timely fashion.

Well, a user able to spawn a WAL sender has replication rights, and it
is already entrusted a lot, particularly knowing that this user can run
BASE_BACKUP and fetch a superuser password which could be used for more
evil actions.  So I am not sure what is actually worrying with this
change in this area, at least it seems to me that the bar is not really
lowered.  An admin can still enforce a value if the client does not
specify it at connection time.  What kind of attack would you see?  An
evil user connecting with a insanely high value and delaying failure
detection, impacting the system performance?
--
Michael


signature.asc
Description: PGP signature

Re: Synchronous replay take III

[Thomas Munro]

On Mon, Jul 2, 2018 at 12:39 PM Thomas Munro
 wrote:
> On Sat, Mar 3, 2018 at 2:11 AM, Adam Brusselback
>  wrote:
> > Thanks Thomas, appreciate the rebase and the work you've done on this.
> > I should have some time to test this out over the weekend.
>
> Rebased.

Rebased.

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


0001-Synchronous-replay-mode-for-avoiding-stale-reads--v7.patch
Description: Binary data

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Andrew Gierth]

> "Tom" == Tom Lane  writes:

 Tom> The FROM FIRST/LAST bit seems particularly badly thought through,
 Tom> because AFAICS it is flat out ambiguous with a normal FROM clause
 Tom> immediately following the window function call. The only way to
 Tom> make it not so would be to make FIRST and LAST be fully reserved,
 Tom> which is neither a good idea nor spec-compliant.

In the actual spec syntax it's not ambiguous at all because NTH_VALUE is
a reserved word (as are LEAD, LAG, FIRST_VALUE and LAST_VALUE), and OVER
is a mandatory clause in its syntax, so a FROM appearing before the OVER
must be part of a FROM FIRST/LAST and not introducing a FROM-clause.

In our syntax, if we made NTH_VALUE etc. a col_name_keyword (and thus
not legal as a function name outside its own special syntax) it would
also become unambiguous.

i.e. given this token sequence (with . marking the current posision):

  select nth_value(x) . from first ignore 

if we know up front that "nth_value" is a window function and not any
other kind of function, we know that we have to shift the "from" rather
than reducing the select-list because we haven't seen an "over" yet.
(Neither "first" nor "ignore" are reserved, so "select foo(x) from first
ignore;" is a valid and complete query, and without reserving the
function name we'd need at least four tokens of lookahead to decide
otherwise.)

This is why I think the col_name_keyword option needs to be given
serious consideration - it still doesn't reserve the names as strongly
as the spec does, but enough to make the standard syntax work without
needing any dubious hacks.

-- 
Andrew (irc:RhodiumToad)

Re: [PATCH] Improve geometric types

[Tomas Vondra]

On 09/03/2018 04:08 AM, Tom Lane wrote:
> Tomas Vondra  writes:
>> On 08/17/2018 11:23 PM, Tom Lane wrote:
>>> Yeah, we've definitely hit such problems before.  The geometric logic
>>> seems particularly prone to it because it's doing more and subtler
>>> float arithmetic than the rest of the system ... but it's not the sole
>>> source of minus-zero issues.
> 
>> We can go through the patch and fix places with obvious -0 risks, but
>> then what? I don't have access to any powepc machines, so I can't check
>> if there are any other failures. So the only thing I could do is commit
>> and fix based on buildfarm failures ...
> 
> That's the usual solution.  I don't see anything particularly wrong
> with it.  If the buildfarm results suggest a whole mess of problems,
> it might be appropriate to revert and rethink; but you shouldn't be
> afraid to use the buildfarm as a testbed.
> 

FWIW I plan to get this committed sometime this week, when I'm available
to fix the expected buildfarm breakage.

regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Collation versioning

[Thomas Munro]

On Tue, Sep 18, 2018 at 7:57 AM Douglas Doole  wrote:
> On Mon, Sep 17, 2018 at 12:32 PM Greg Stark  wrote:
>> This seems like a terrible idea in the open source world. Surely collation 
>> versioning means new ICU libraries can still provide the old collation rules 
>> so even if you update the library you can request the old version? We 
>> shouldn't need that actual old code with all its security holes and bugs 
>> just to get the old collation version.
>
>
> We asked long and hard for this feature from the ICU team but they kept 
> arguing it was too hard to do. There are apparently some tight couplings 
> between the code and each version of CLDR. So the only way to support old 
> collations is to ship the entire old library. (They even added make rules to 
> allow the entire API to be version extended to accommodate this requirement.)

I wonder if this would be best modelled by entirely separate collation
entries with different OIDs, and possibly also separate collation
providers.  Considering that to handle this we'd need to figure out
how link libicu.so.55, libicu.so.56, ... etc into the same backend,
and yet they presumably have the same collation names, doing it as
separate providers would create separate namespaces for their
collcollate values and reflect the reality that they really are
entirely independent providers.  Admittedly that creates a whole can
of worms for initdb-time catalog creation, package maintainers' jobs,
how long old versions have to be supported and how you upgraded
database objects to new ICU versions.  This kind of "major" versioning
with support for concurrently accessible major versions seems to be
different from the kind of version changes that happen under your feet
when libraries/collation definition files are updated.

> Even bug fixes are potentially problematic because the fix may alter how some 
> code points collate. The ICU team won't (or at least wouldn't - been a few 
> years since I dealt with them) guarantee any sort of backwards compatibility 
> between code drops.

Yeah, it seems like ICU is *also* subject to minor changes that happen
under your feet, much like libc.  For example maintenance release 60.2
(you can't install that at the same time as 60.1, but you can install
it at the same time as 59.2).  You'd be linked against libicu.so.60
(and thence libicudata.so.60), and it gets upgraded in place when you
run the local equivalent of apt-get upgrade.

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

Re: Missing const in DSA.

[Tom Lane]

Mark G  writes:
> While looking at some of the recent churn in DSA I noticed that
> dsa_size_class_map should probably be declared const.

+1 ... also, given the contents of the array, "char" seems like
rather a misnomer.  I'd be happier if it were declared as uint8, say.

regards, tom lane

Re: Add RESPECT/IGNORE NULLS and FROM FIRST/LAST options

[Tom Lane]

Andrew Gierth  writes:
> Normally I'd push hard to try and get some solution that's sufficiently
> generic to allow user-defined functions to make use of the feature. But
> I think the SQL spec people have managed to make that literally
> impossible in this case, what with the FROM keyword appearing in the
> middle of a production and not followed by anything sufficiently
> distinctive to even use for extra token lookahead.

Yeah.  Is there any appetite for a "Just Say No" approach?  That is,
refuse to implement the spec's syntax on the grounds that it's too
brain-dead to even consider, and instead provide some less random,
more extensibility-friendly way to accomplish the same thing?

The FROM FIRST/LAST bit seems particularly badly thought through,
because AFAICS it is flat out ambiguous with a normal FROM clause
immediately following the window function call.  The only way to
make it not so would be to make FIRST and LAST be fully reserved,
which is neither a good idea nor spec-compliant.

In short, there's a really good case to be made here that the SQL
committee is completely clueless about syntax design, and so we
shouldn't follow this particular pied piper.

> ... This patch would make lead / lag /
> first_value / last_value / nth_value syntactically "special" while not
> actually reserving them (beyond having them in unreserved_keywords); I
> think serious consideration should be given to whether they should
> instead become col_name_keywords (which would, I believe, make it
> unnecessary to mess with precedence).

I agree that messing with the precedence rules is likely to have
unforeseen and undesirable side-effects.  Generally, if you need
to create a precedence rule, that's because your grammar is
ambiguous.  Precedence fixes that in a well-behaved way only for
cases that actually are very much like operator precedence rules.
Otherwise, you may just be papering over something that isn't
working very well.  See e.g. commits 670a6c7a2 and 12b716457
for past cases where we learned that the hard way.  (The latter
also points out that if you must have a precedence hack, it's
safer to hack individual rules than to stick precedences onto
terminal symbols.)  In the case at hand, since the proposed patch
doesn't make FIRST and LAST be fully reserved, it seems just about
certain that it can be made to misbehave, including failing on
queries that were and should remain legal.

regards, tom lane

Re: Pluggable Storage - Andres's take

[Alexander Korotkov]

On Fri, Aug 24, 2018 at 5:50 AM Andres Freund  wrote:
> I've pushed a current version of that to my git tree to the
> pluggable-storage branch. It's not really a version that I think makese
> sense to review or such, but it's probably more useful if you work based
> on that.  There's also the pluggable-zheap branch, which I found
> extremely useful to develop against.

BTW, I'm going to take a look at current shape of this patch and share
my thoughts.  But where are the branches you're referring?  On your
postgres.org git repository pluggable-storage brach was updates last
time at June 7.  And on the github branches are updated at August 5
and 14, and that is still much older than your email (August 24)...

1. 
https://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=shortlog;h=refs/heads/pluggable-storage
2. https://github.com/anarazel/postgres-pluggable-storage
3, https://github.com/anarazel/postgres-pluggable-storage/tree/pluggable-zheap

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: testing pg_dump against very old versions

[Andrew Dunstan]

On 09/22/2018 12:46 PM, Andrew Dunstan wrote:


In the interest of advancing $subject, I recently started a little 
skunkworks project to get old postgres running on modern systems so we 
could test if we'd broken backwards compatibility somehow. This was 
given a fillip a few days ago when my colleague Gianni Ciolli 
complained that it uses array syntax that isn't valid in 7.3 for the 
-T option. So here is the result. Essentially I set up a (barely 
workable) Fedora Core 2 VM and build Postgres 7.2.8 there. Then I 
packed up the binaries and data directory and tried them on a modern 
system (Fedora 28). It turns out they need a few old libraries, but 
apart from that it works. So I have packaged all this up in a Vagrant 
setup, which is available at 


Next I'm going to work on a Docker image for this. I think that should 
be fairly simple now I have this piece down.






OK, the repo now has a Dockerfile, and I have pushed a Docker image to 
dockerhub. It can be pulled as andrewdunstan/postgres:pg7.2.8 That 
should make it easy enough for many people to play with.


For testing, I think we'll need a way to specify where to connect to. 
One simple way might be to set an environnment variable, like 
REL7_2_TEST_CONNINFO="hostname=192.168.129.156 port=5478 user=postgres". 
But ideally we want more than that - possibly some pre and post test 
commands, such as "ssh dockerhost docker run -d --name pg72 -p 5478:5432 
mycontainer" and "ssh dockerhost kill pg72". this would be easily 
manageable in a buildfarm context, not sure how we should manage it in 
core code, though.


cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
q

Re: Changing the setting of wal_sender_timeout per standby

[Tom Lane]

Andres Freund  writes:
> Have there been discussions about the security effects of this change?
> Previously the server admin could control the timeout, which could
> affect things like syncrep, after this it's not possible anymore.  I
> *think* that's ok, but it should be discussed.

Hm.  An evil replication connection could already cause all sorts of
operational problems (and I'm not counting grabbing all your data).
Does this add anything much new in that line?  It seems like the
effects would be at least in the same ballpark as not sending
hot-standby-feedback messages in a timely fashion.

regards, tom lane

Re: Collation versioning

[Thomas Munro]

On Wed, Sep 19, 2018 at 1:16 PM Stephen Frost  wrote:
> * Thomas Munro (thomas.mu...@enterprisedb.com) wrote:
> > On Wed, Sep 19, 2018 at 10:09 AM Stephen Frost  wrote:
> > > * Douglas Doole (dougdo...@gmail.com) wrote:
> > > > > The CHECK constraint doesn't need to directly track that information-
> > > > > it should have a dependency on the column in the table and that's 
> > > > > where
> > > > > the information would be recorded about the current collation version.
> > > >
> > > > Just to have fun throwing odd cases out, how would something like this 
> > > > be
> > > > recorded?
> > > >
> > > > Database default collation: en_US
> > > >
> > > > CREATE TABLE t (c1 TEXT, c2 TEXT, c3 TEXT,
> > > >  CHECK (c1 COLLATE "fr_FR" BETWEEN c2 COLLATE "fr_FR" AND c3 COLLATE
> > > > "fr_FR"));
> > > >
> > > > You could even be really warped and apply multiple collations on a 
> > > > single
> > > > column in a single constraint.
> > >
> > > Once it gets to an expression and not just a simple check, I'd think
> > > we'd record it in the expression..
> >
> > Maybe I misunderstood, but I don't think it makes sense to have a
> > collation version "on the column in the table", because (1) that fails
> > to capture the fact that two CHECK constraints that were defined at
> > different times might have become dependent on two different versions
> > (you created one constraint before upgrading and the other after, now
> > the older one is invalidated and sounds the alarm but the second one
> > is fine), and (2) the table itself doesn't care about collation
> > versions since heap tables are unordered; there is no particular
> > operation on the table that would be the correct time to update the
> > collation version on a table/column.  What we're trying to track is
> > when objects that in some way depend on the version become
> > invalidated, so wherever we store it there's going to have to be a
> > version recorded per dependent object at its creation time, so that's
> > either new columns on every interested catalog table, or ...
>
> Today, we work out what operators to use for a given column based on the
> data type.  This is why I was trying to get at the idea of using typmod
> earlier, but if we can't make that work then I'm inclined to try and
> figure out a way to get it as close as possible to being associated with
> the type.
>
> Yes, the heap is unordered, but the type *is* ordered and we track that
> with the type system.  Maybe we just extend that somehow, rather than
> using the typmod or adding columns into catalogs where we store type
> information (such as pg_attribute).  Perhaps typmod could be made larger
> to be able to support this, that might be another approach.

Can you show how this would look in the catalogs, for Doug's example
above?  There is no pg_attribute or similar that applies here.
Perhaps you want to bury the versions inside the serialised expression
tree?  Or store it in a pair of arrays on the pg_constraint row?

> No where in this does it strike me that it makes sense to push this into
> pg_depend though.

In my scheme, the pg_depend row that already exists to record the
dependency between the pg_constraint row and the pg_collation row for
"fr_FR" holds a refobjversion value captured at constraint creation
time.  Maybe that's a bit strange, but the alternatives I have thought
of so far seemed ad hoc and strange too.

Here's a Sunday afternoon rapid prototype of the concept, implemented
for indexes and check constraints.  Examples of the output you get
when you access those database objects for the first time in each
backend:

WARNING:  index "foo_i_idx" depends on collation 16385 version
"30.0.1", but the current version is "30.0.2"
DETAIL:  The index may be corrupted due to changes in sort order.
HINT:  REINDEX to avoid the risk of corruption.

WARNING:  constraint "t_i_check" depends on collation 12018 version
"30.0.1", but the current version is "30.0.2"
DETAIL:  The constraint may be corrupted due to changes in sort order.
HINT:  Drop and recreate the constraint to avoid the risk of corruption.

I didn't try to tackle the default collation yet, so to try this you
need to use non-default collations.

Whether we'd actually want to do this for CHECK constraints or any of
this, I don't know.  It's theoretically the right thing to do, but
most people probably don't want to be nagged to recreate (or recheck
in some new way) all their constraints every time they do an OS
upgrade, and in fact there is almost never anything actually wrong if
they don't... unless they're unlucky.  But I wanted to do this to show
that it's a fairly general way to deal with dependencies between
database objects and collation versions using the existing links
between them in a relational sort of way.  I guess you might want a
command or tool to find all such problems proactively and recheck,
reindex, repartition etc as required to make it shut up, and that
should be quite easy to drive from this design.

[patch]overallocate memory for curly braces in array_out

[Keiichi Hirobe]

Hi,

Attached is a patch that fixes a bug
for miscounting total number of curly braces in output string in array_out.

Example1
{{1,2,3},{4,5,6}} -> dims[0] = 2, dims[1]= 3

- Without the patch
8
- After
3

Example2
N size one-dimensional array -> dims[0] = N

- Without the patch
N
- After
1

I used gdb to confirm the above behavior.

Cheers,
Keiichi Hirobe


array_out_bugfix.patch
Description: Binary data

amcheck verification for GiST

[Andrey Borodin]

Hi, hackers!

Here's the patch with amcheck functionality for GiST.

It basically checks two invariants:
1. Every internal tuple need no adjustment by tuples of referenced page
2. Internal page reference or only leaf pages or only internal pages

We actually cannot check all balanced tree invariants due to concurrency 
reasons some concurrent splits will be visible as temporary balance violations.

Are there any other invariants that we can check?

I'd be happy to hear any thought about this.

Best regards, Andrey Borodin.


0001-GiST-verification-function-for-amcheck.patch
Description: Binary data

Missing const in DSA.

[Mark G]

While looking at some of the recent churn in DSA I noticed that
dsa_size_class_map should probably be declared const.
diff --git a/src/backend/utils/mmgr/dsa.c b/src/backend/utils/mmgr/dsa.c
index 33ab8d05d3..70ce7ce7da 100644
--- a/src/backend/utils/mmgr/dsa.c
+++ b/src/backend/utils/mmgr/dsa.c
@@ -256,7 +256,7 @@ static const uint16 dsa_size_classes[] = {
  * round the size of the object up to the next multiple of 8 bytes, and then
  * index into this array.
  */
-static char dsa_size_class_map[] = {
+static const char dsa_size_class_map[] = {
 	2, 3, 4, 5, 6, 7, 8, 9, 10, 10, 11, 11, 12, 12, 13, 13,
 	14, 14, 14, 14, 15, 15, 15, 15, 16, 16, 16, 16, 17, 17, 17, 17,
 	18, 18, 18, 18, 18, 18, 18, 18, 19, 19, 19, 19, 19, 19, 19, 19,