Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

(2018/01/13 6:07), Robert Haas wrote:

On Sun, Dec 3, 2017 at 2:56 PM, Tom Lane  wrote:

I thought some more about this.  While it seems clear that we don't
actually need to recheck anything within a postgres_fdw foreign join,
there's still a problem: we have to be able to regurgitate the join
row during postgresRecheckForeignScan.  Since, in the current system
design, the only available data is scan-level rowmarks (that is,
whole-row values from each base foreign relation), there isn't any
very good way to produce the join row except to insert the rowmark
values into dummy scan nodes and then re-execute the join locally.
So really, that is what the outerPlan infrastructure is doing for us.


I started to look at this patch again today and got cold feet.  It
seems to me that this entire design on the posted patch is based on
your remarks in http://postgr.es/m/13242.1481582...@sss.pgh.pa.us --

# One way that we could make things better is to rely on the knowledge
# that EPQ isn't asked to evaluate joins for more than one row per input
# relation, and therefore using merge or hash join technology is really
# overkill.  We could make a tree of simple nestloop joins, which aren't
# going to care about sort order, if we could identify the correct join
# clauses to apply.


That's right.


However, those remarks are almost entirely wrong.  It's true that the
relations for which we have EPQ tuples will only ever output a single
tuple, but because of what you said in the quoted text above, this
does NOT imply that the recheck plan never needs to worry about
dealing with a large number of rows, as the following example shows.
(The part about making a tree of simple nestloop joins is wrong not
only for this reason but because it won't work in the executor, as
previously discussed.)

rhaas=# explain select * from foo join (bar join baz on bar.a = baz.a)
on foo.a = bar.a where foo.b = 'hello' for update;
   QUERY PLAN
---
  LockRows  (cost=109.02..121.46 rows=1 width=162)
->   Merge Join  (cost=109.02..121.45 rows=1 width=162)
  Merge Cond: (bar.a = foo.a)
  ->   Foreign Scan  (cost=100.58..4914.58 rows=4 width=119)
Relations: (public.bar) INNER JOIN (public.baz)
->   Hash Join  (cost=2556.00..4962.00 rows=4 width=119)
  Hash Cond: (bar.a = baz.a)
  ->   Foreign Scan on bar  (cost=100.00..1956.00
rows=4 width=28)
  ->   Hash  (cost=1956.00..1956.00 rows=4 width=27)
->   Foreign Scan on baz
(cost=100.00..1956.00 rows=4 width=27)
  ->   Sort  (cost=8.44..8.45 rows=1 width=28)
Sort Key: foo.a
->   Index Scan using foo_b_idx on foo  (cost=0.41..8.43
rows=1 width=28)
  Index Cond: (b = 'hello'::text)

There's really nothing to prevent the bar/baz join from producing
arbitrarily many rows, which means that it's not good to turn the
recheck plan into a Nested Loop unconditionally -- and come to think
of it, I'm pretty sure this kind of case is why
GetExistingLocalJoinPath() tries to using an existing path: it wants a
path that isn't going suck if it gets executed.


Yeah, but I don't think the above example is good enough to explain 
that, because I think the bar/baz join would produce at most one tuple 
in an EPQ recheck since we would have only one EPQ tuple for both bar 
and baz in that recheck, and the join is inner.  I think such an example 
would probably be given e.g., by a modified version of the SQL where we 
have a full join of bar and baz, not an inner join.



After some thought, it seems that there's a much simpler way that we
could fix the problem you identified in that original email: if the
EPQ path isn't properly sorted, have postgres_fdw's
add_paths_with_pathkeys_for_rel stick a Sort node on top of it.  I
tried this and it does indeed fix Jeff Janes' initial test case.
Patch attached.


The patch looks good to me.


One could do better here if one wanted to revise
GetExistingLocalJoinPath() to take Last *pathkeys as an additional
argument; we could then try to find the optimal EPQ path for each
possible set of sortkeys.  But I don't feel the need to work on that
right now, and it would be nicer not to back-patch a change to the
signature of GetExistingLocalJoinPath(), as has already been noted.


That makes sense to me.


The only problem that
*has actually been demonstrated* is that we can end up using as an EPQ
path something that doesn't generate the right sort order, and that is
easily fixed by making it do so, which the attached patch does.  So I
propose we commit and back-patch this and move on.


Agreed.

Best regards,
Etsuro Fujita

Re: [HACKERS] Useless code in ExecInitModifyTable

[Etsuro Fujita]

(2018/01/15 11:35), Amit Langote wrote:

On 2018/01/15 11:28, Stephen Frost wrote:

Seems like this has gotten a review (and quite a bit of down-stream
discussion that seemed pretty positive), and the latest patch still
applies cleanly and passes the regression tests- is there some reason
it's still marked as Needs Review?  Seems like it should really be in
Ready for Committer state.


Thanks for taking care of this, Stephen!


+1.


Amit, if you agree, would you mind going ahead and changing it?


Sure, done.


Thanks for reviewing, Amit!

Best regards,
Etsuro Fujita

Re: Removing WITH clause support in CREATE FUNCTION, for isCachable and isStrict

[Daniel Gustafsson]

> On 15 Jan 2018, at 03:27, Michael Paquier  wrote:
> 
> Hi all,
> 
> As noticed by Daniel here:
> https://www.postgresql.org/message-id/d5f34c9d-3ab7-4419-af2e-12f67581d...@yesql.se

In that thread I proposed a patch to fix this, but I certainly don’t object to
just removing it to make both syntax and code clearer. +1

cheers ./daniel

Re: Minor fix for pgbench documentation

[Ildar Musin]

Hello Fabien,


13/01/2018 19:30, Fabien COELHO пишет:
>
>> Here is a patch that adds missing random_zipfian func to the paragraph
>> in pgbench documentation about random functions parameterization.
>
> Indeed.
>
> Patch applies cleanly, doc build ok. Marked as "ready".
>
> I have added it to the next commitfest.
>
Thank you for your review!

-- 
Ildar Musin
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company 

Re: [HACKERS] UPDATE of partition key

[Amit Khandekar]

On 14 January 2018 at 17:27, Amit Khandekar  wrote:
> On 13 January 2018 at 02:56, Robert Haas  wrote:
> > I guess I'm inclined to keep mt_per_sub_plan_maps for the case where
> > there are no partitions, but not use it when partitions are present.
> > What do you think about that?
>
> Even where partitions are present, in the usual case where there are no 
> transition tables we won't require per-leaf map at all [1]. So I think we 
> should keep mt_per_sub_plan_maps only for the case where per-leaf map is not 
> allocated. And we will not allocate mt_per_sub_plan_maps when 
> mt_per_leaf_maps is needed. In other words, exactly one of the two maps will 
> be allocated.
>
> This is turning out to be close to what's already there in the last patch 
> versions: use a single map array, and an offsets array. The difference is : 
> in the patch I am using the *same* variable for the two maps. Where as, now 
> we are talking about two different array variables for maps, but only 
> allocating one of them.
>
> Are you ok with this ? I think the thing you were against was to have a 
> common *variable* for two purposes. But above, I am saying we have two 
> variables but assign a map array to only *one* of them and leave the other 
> unused.
>
> -
>
> Regarding the on-demand map allocation 
> Where mt_per_sub_plan_maps is allocated, we won't have the on-demand 
> allocation: all the maps will be allocated initially. The reason is becaues 
> the map_is_required array is only per-leaf. Or else, again, we need to keep 
> another map_is_required array for per-subplan. May be we can support the 
> on-demand stuff for subplan maps also, but only as a separate change after we 
> are done with update-partition-key.
>
>
> -
>
> Regarding mt_per_leaf_tupconv_required, I am thinking we can make it a bool, 
> and name it : mt_per_leaf_map_not_required. When it is true for a given 
> index, it means, we have already called convert_tuples_by_name() and it 
> returned NULL; i.e. it means we are sure that map is not required. A false 
> value means we need to call convert_tuples_by_name() if it is NULL, and then 
> set mt_per_leaf_map_not_required to (map == NULL).
>
> Instead of a bool array, , we can instead make it a Bitmapset. But I think 
> access would become slower as compared to array, particularly because it is 
> going to be a heavily used function.

I went ahead and did the above changes. I haven't yet merged these
changes in the main patch. Instead, I have attached it as an
incremental patch to be applied on the main v36 patch. The incremental
patch is not yet quite polished, and quite a bit of cosmetic changes
might be required, plus testing. But am posting it in case I have some
early feedback. Details :

The per-subplan map array variable is kept in ModifyTableState :
-   TupleConversionMap **mt_childparent_tupconv_maps;
-   /* Per plan/partition map for tuple conversion from child to root */
-   boolmt_is_tupconv_perpart;  /* Is the above map
per-partition ? */
+   TupleConversionMap **mt_per_subplan_tupconv_maps;
+   /* Per plan map for tuple conversion from child to root */
 } ModifyTableState;

The per-leaf array variable and the not_required array is kept in
PartitionTupleRouting :
-   TupleConversionMap **partition_tupconv_maps;
+   TupleConversionMap **parent_child_tupconv_maps;
+   TupleConversionMap **child_parent_tupconv_maps;
+   bool   *child_parent_tupconv_map_not_reqd;
As you can see above, all the arrays are per-partition. So removed the
per-leaf tag in these arrays. Instead, renamed the existing
partition_tupconv_maps to parent_child_tupconv_maps, and the new
per-leaf array to child_parent_tupconv_maps

Have two separate functions ExecSetupChildParentMapForLeaf() and
ExecSetupChildParentMapForSubplan() since most of their code is
different. And now because of this, we can re-use
ExecSetupChildParentMapForLeaf() in both copy.c and nodeModifyTable.c.

Even inserts/copy will benefit from the on-demand map allocation. This
is because now there is a function TupConvMapForLeaf() that is called
in both copy.c and ExecInsert(). This is the function that does
on-demand allocation.

Attached the incremental patch conversion_map_changes.patch that has
the above changes. It is to be applied over the latest main patch
(update-partition-key_v36.patch).


conversion_map_changes.patch
Description: Binary data

Re: [PATCH] Logical decoding of TRUNCATE

[Marco Nenciarini]

Hi all,

After commit bbd3363e128daec0e70952c1bb2f12ab1f6f1292 that refactor
subscription tests to use PostgresNode's wait_for_catchup, the patch
needs to be updated to use wait_for_catchup.

Attached there is the updated patch. is discussed in a separate thread
https://commitfest.postgresql.org/16/1447/, but I've posted it also here
to ease the review of the 0002.

Regards,
Marco

-- 
Marco Nenciarini - 2ndQuadrant Italy
PostgreSQL Training, Services and Support
marco.nenciar...@2ndquadrant.it | www.2ndQuadrant.it
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index e4a01699e4..aff56891e6 100644
*** a/doc/src/sgml/config.sgml
--- b/doc/src/sgml/config.sgml
***
*** 6502,6508  COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv;

 
  Controls firing of replication-related triggers and rules for the
! current session.  Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
--- 6502,6510 

 
  Controls firing of replication-related triggers and rules for the
! current session. When set to replica it also
! disables all the foreign key checks, which can leave the data in an
! inconsistent state if improperly used. Setting this variable requires
  superuser privilege and results in discarding any previously cached
  query plans.  Possible values are origin (the 
default),
  replica and local.
diff --git a/src/backend/commanindex f2a928b823..180ebd0717 100644
*** a/src/backend/commands/tablecmds.c
--- b/src/backend/commands/tablecmds.c
***
*** 1340,1355  ExecuteTruncate(TruncateStmt *stmt)
}
  
/*
!* Check foreign key references.  In CASCADE mode, this should be
!* unnecessary since we just pulled in all the references; but as a
!* cross-check, do it anyway if in an Assert-enabled build.
 */
  #ifdef USE_ASSERT_CHECKING
-   heap_truncate_check_FKs(rels, false);
- #else
-   if (stmt->behavior == DROP_RESTRICT)
heap_truncate_check_FKs(rels, false);
  #endif
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
--- 1340,1363 
}
  
/*
!* Suppress foreign key references check if session replication role is
!* set to REPLICA.
 */
+   if (SessionReplicationRole != SESSION_REPLICATION_ROLE_REPLICA)
+   {
+ 
+   /*
+* Check foreign key references.  In CASCADE mode, this should 
be
+* unnecessary since we just pulled in all the references; but 
as a
+* cross-check, do it anyway if in an Assert-enabled build.
+*/
  #ifdef USE_ASSERT_CHECKING
heap_truncate_check_FKs(rels, false);
+ #else
+   if (stmt->behavior == DROP_RESTRICT)
+   heap_truncate_check_FKs(rels, false);
  #endif
+   }
  
/*
 * If we are asked to restart sequences, find all the sequences, lock 
them
diff --git a/contrib/test_decoding/expected/ddl.out 
b/contrib/test_decoding/expected/ddl.out
index 1e22c1eefc..d1feea4909 100644
*** a/contrib/test_decoding/expected/ddl.out
--- b/contrib/test_decoding/expected/ddl.out
***
*** 543,548  UPDATE table_with_unique_not_null SET data = 3 WHERE data = 2;
--- 543,550 
  UPDATE table_with_unique_not_null SET id = -id;
  UPDATE table_with_unique_not_null SET id = -id;
  DELETE FROM table_with_unique_not_null WHERE data = 3;
+ TRUNCATE table_with_unique_not_null;
+ TRUNCATE table_with_unique_not_null, table_with_unique_not_null RESTART 
IDENTITY CASCADE;
  -- check toast support
  BEGIN;
  CREATE SEQUENCE toasttable_rand_seq START 79 INCREMENT 1499; -- portable 
"random"
***
*** 660,665  SELECT data FROM 
pg_logical_slot_get_changes('regression_slot', NULL, NULL, 'inc
--- 662,673 
   table public.table_with_unique_not_null: DELETE: id[integer]:4
   COMMIT
   BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: (no-flags)
+  COMMIT
+  BEGIN
+  table public.table_with_unique_not_null: TRUNCATE: restart_seqs cascade
+  COMMIT
+  BEGIN
   table public.toasttable: INSERT: id[integer]:1 
toasted_col1[text]:'12345678910111213141516171819202122232425262728293031323334353637383940414243444546474849505152535455565758596061626364656667686970717273747576777879808182838485868788899091929394959697989910010110210310410510610710810911012113114115116117118119120121122123124125126127128129130131132133134135136137138139140141142143144145146147148149150151152153154155156157158159160161162163164165166167168169170171172173174175176177178179180181182183184185186187188189190191192193194195196197198199200201202203204205206207208209210211212213214215216217218219220221232242252262

Re: master make check fails on Solaris 10

[Marina Polyakova]

Thank you very much!

On 13-01-2018 21:10, Tom Lane wrote:

Marina Polyakova  writes:

On 12-01-2018 21:00, Tom Lane wrote:

Hm ... so apparently, that compiler has bugs in handling nondefault
alignment specs.  You said upthread it was gcc, but what version
exactly?



This is 5.2.0:


Ugh ... protosciurus has 3.4.3, but I see that configure detects that
as *not* having __int128.  Probably what's happening on your machine
is that gcc knows __int128 but generates buggy code for it when an
alignment spec is given.  So that's unfortunate, but it's not really
a regression from 3.4.3.

I'm not sure there's much we can do about this.  Dropping the use
of the alignment spec isn't a workable option.  If there were a
simple way for configure to detect that the compiler generates bad
code for that, we could have it do so and reject use of __int128,
but it'd be up to you to come up with a workable test.


I'll think about it..


In the end this might just be an instance of the old saw about
avoiding dot-zero releases.  Have you tried a newer gcc?
(Digging in their bugzilla finds quite a number of __int128 bugs
fixed in 5.4.x, though none look to be specifically about
misaligned data.)


As I was told offlist, 5.2.0 is already a fairly new version of gcc for 
this system..



Also, if it still happens with current gcc on that hardware,
there'd be grounds for a new bug report to them.


--
Marina Polyakova
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: [HACKERS] UPDATE of partition key

[Robert Haas]

On Sun, Jan 14, 2018 at 6:57 AM, Amit Khandekar  wrote:
> Even where partitions are present, in the usual case where there are
> no transition tables we won't require per-leaf map at all [1]. So I
> think we should keep mt_per_sub_plan_maps only for the case where
> per-leaf map is not allocated. And we will not allocate
> mt_per_sub_plan_maps when mt_per_leaf_maps is needed. In other words,
> exactly one of the two maps will be allocated.
>
> This is turning out to be close to what's already there in the last
> patch versions: use a single map array, and an offsets array. The
> difference is : in the patch I am using the *same* variable for the
> two maps. Where as, now we are talking about two different array
> variables for maps, but only allocating one of them.
>
> Are you ok with this ? I think the thing you were against was to have
> a common *variable* for two purposes. But above, I am saying we have
> two variables but assign a map array to only *one* of them and leave
> the other unused.

Yes, I'm OK with that.

> Regarding the on-demand map allocation 
> Where mt_per_sub_plan_maps is allocated, we won't have the on-demand
> allocation: all the maps will be allocated initially. The reason is
> becaues the map_is_required array is only per-leaf. Or else, again, we
> need to keep another map_is_required array for per-subplan. May be we
> can support the on-demand stuff for subplan maps also, but only as a
> separate change after we are done with update-partition-key.

Sure.

> Regarding mt_per_leaf_tupconv_required, I am thinking we can make it a
> bool array, and name it : mt_per_leaf_map_not_required. When it is
> true for a given index, it means, we have already called
> convert_tuples_by_name() and it returned NULL; i.e. it means we are
> sure that map is not required. A false value means we need to call
> convert_tuples_by_name() if it is NULL, and then set
> mt_per_leaf_map_not_required to (map == NULL).

OK.

> Instead of a bool array, we can even make it a Bitmapset. But I think
> access would become slower as compared to array, particularly because
> it is going to be a heavily used function.

It probably makes little difference -- the Bitmapset will be more
compact (which saves time) but involve function calls (which cost
time).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Implementing SQL ASSERTION

[Fabien COELHO]

Hello Joe,

Just a reaction to the example, which is maybe addressed in the patch 
which I have not investigated.


* certain combinations of aggregates with comparison operations cannot 
be invalidating.


As an example of the last point, the expression "CHECK (10 > (SELECT 
COUNT(*) FROM t))" cannot be invalidated by a delete or an update but 
can be invalidated by an insert.


I'm wondering about the effect of MVVC on this: if the check is performed 
when the INSERT is done, concurrent inserting transactions would count the 
current status which would be ok, but on commit all concurrent inserts 
would be there and the count could not be ok anymore?


Maybe if the check was deferred, but this is not currently possible with 
pg (eg the select can simply be put in a function), and I there might be 
race conditions. ISTM that such a check would imply non trivial locking to 
be okay, it is not just a matter of deciding whether to invoke the check 
or not.


--
Fabien.

Re: proposal: alternative psql commands quit and exit

[Laurenz Albe]

Everaldo Canuto wrote:
> Also I don't see a consensus on this thread and I don't understand how 
> decisions are taken.

It's just difficult to find consensus with many people.

There were several valid concerns with this, the most striking (to me)
was Tome's concern that there are perfectly valid multi-line SQL statements
where a line could start with "exit" or "quit".

Robert Haas had an idea how to provide useful hints without breaking anything,
but it seemed a little complicated.

There was some discussion whether the lack of "quit" or "exit" is a real problem
or not, but most people could share your view that it would be good to improve
the newcomers' experience.


Consensus is never reached explicitly, rather implicitly through positive
feedback and a lack of concerns.

Everybody can have an opinion, but in a "meritocracy" like this not all opinions
are equal.  If a "notable" contributor has an objection, and no person of
equal standing disagrees, you have to either address the concerns or consider
the proposal rejected.


If you see a way forward, describe it to attract further feedback.
If you feel that you have enough buy-in,  you can add the patch to the next
commitfest.  The ultimate test is then if you can attract reviewers that
examine your patch and committers that are willing to spend time to
commit it.


I am aware that this process can be quite tiresome and discouraging, but
any patch that makes it through will be much improved by it.

Yours,
Laurenz Albe

Re: Implementing SQL ASSERTION

[Joe Wildish]

Hi Fabien,

>> * certain combinations of aggregates with comparison operations cannot be 
>> invalidating.
>> 
>> As an example of the last point, the expression "CHECK (10 > (SELECT 
>> COUNT(*) FROM t))" cannot be invalidated by a delete or an update but can be 
>> invalidated by an insert.
> 
> I'm wondering about the effect of MVVC on this: if the check is performed 
> when the INSERT is done, concurrent inserting transactions would count the 
> current status which would be ok, but on commit all concurrent inserts would 
> be there and the count could not be ok anymore?

Yes, there was quite a bit of discussion in the original thread about 
concurrency. See here:

https://www.postgresql.org/message-id/flat/1384486216.5008.17.camel%40vanquo.pezone.net#1384486216.5008.17.ca...@vanquo.pezone.net
 


The patch doesn’t attempt to address concurrency (beyond the obvious benefit of 
reducing the circumstances under which the assertion is checked). I am working 
under the assumption that we will find some acceptable way for that to be 
resolved :-) And at the moment, working in serialisable mode addresses this 
issue. I think that is suggested in the thread actually (essentially, if you 
want to use assertions, you require that transactions be performed at 
serialisable isolation level). 

> Maybe if the check was deferred, but this is not currently possible with pg 
> (eg the select can simply be put in a function), and I there might be race 
> conditions. ISTM that such a check would imply non trivial locking to be 
> okay, it is not just a matter of deciding whether to invoke the check or not.

I traverse into SQL functions so that the analysis can capture invalidating 
operations from the expression inside the function. Only internal and SQL 
functions are considered legal. Other languages are rejected.

-Joe

Re: new function for tsquery creartion

[Aleksander Alekseev]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Here are a few minor issues:

```
+/*
+ * Checks if 'str' starts with a 'prefix'
+ */
+static bool
+has_prefix(char * str, char * prefix)
+{
+   if (strlen(prefix) > strlen(str))
+   {
+   return false;
+   }
+   return strstr(str, prefix) == str;
+}
```

strlen() check is redundant.

```
+   case OP_AROUND:
+   snprintf(in->cur, 256, " AROUND(%d) %s", distance, nrm.buf);
+   break;
```

Instead of the constant 256 it's better to use sizeof().

Apart from these issues this patch looks not bad.

The new status of this patch is: Ready for Committer

Re: WIP: a way forward on bootstrap data

[Alvaro Herrera]

Tom Lane wrote:

> I had a thought about how to do that.  It's clearly desirable that that
> sort of material remain in the manually-maintained pg_*.h files, because
> that's basically where you look to find out C-level details of what's
> in a particular catalog.  However, that doesn't mean that that's where
> the compiler has to find it.
>
> [ elided explanation of pg_foo_d.h and pg_foo.h ]

Sounds good to me.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Implementing SQL ASSERTION

[Fabien COELHO]

I'm wondering about the effect of MVVC on this: if the check is 
performed when the INSERT is done, concurrent inserting transactions 
would count the current status which would be ok, but on commit all 
concurrent inserts would be there and the count could not be ok 
anymore?


The patch doesn’t attempt to address concurrency (beyond the obvious 
benefit of reducing the circumstances under which the assertion is 
checked). I am working under the assumption that we will find some 
acceptable way for that to be resolved :-) And at the moment, working in 
serialisable mode addresses this issue. I think that is suggested in the 
thread actually (essentially, if you want to use assertions, you require 
that transactions be performed at serialisable isolation level).


Thanks for the pointers. The "serializable" isolation level restriction 
sounds reasonnable.


--
Fabien.

Re: WIP Patch: Precalculate stable functions, infrastructure v1

[Aleksander Alekseev]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

I can confirm this code works. However, since this is quite a large patch, I 
believe we better have a second reviewer or a very attentive committer. 

The new status of this patch is: Ready for Committer

Re: [HACKERS] GSoC 2017: weekly progress reports (week 4) and patch for hash index

[Shubham Barai]

On 15 January 2018 at 08:03, Stephen Frost  wrote:

> Greeting Shubham, all,
>
> * Michael Paquier (michael.paqu...@gmail.com) wrote:
> > On Mon, Sep 25, 2017 at 10:34 PM, Shubham Barai
> >  wrote:
> > > I have attached the rebased version of patch here.
> >
> > The patch does not apply and there has been no reviews as well. In
> > consequence, I am moving it to next CF with "waiting on author" as
> > status. Please provide a rebased patch.
>
> Shubham, would you mind providing an updated patch which applies
> cleanly, so we can change this to Needs Review and hopefully get someone
> looking at it?  Also, it would be good to respond to Alexander's as to
> if it would work or not (and perhaps updating the patch accordingly).
> Otherwise, I'm afriad this patch may end up just getting bumped to the
> next CF or ending up as 'returned with feedback'.  Would be great to get
> this up to a point where it could be committed.
>
>
>
Hi Stephen,

The new approach was suggested after completion of GSoC. So, I didn't get
enough time to implement this approach. Also, I was constantly updating my
patches for gist and gin index based on reviews.

Here, I am attaching the rebased version of the patch (which is based on an
old approch:
acquiring a predicate lock on primary page of a bucket)

Kind Regards,
Shubham


Predicate-Locking-in-hash-index_v5.patch
Description: Binary data

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Mon, Jan 15, 2018 at 7:28 AM, Laurenz Albe  wrote:
> Everaldo Canuto wrote:
>> Also I don't see a consensus on this thread and I don't understand how 
>> decisions are taken.
>
> It's just difficult to find consensus with many people.
>
> There were several valid concerns with this, the most striking (to me)
> was Tome's concern that there are perfectly valid multi-line SQL statements
> where a line could start with "exit" or "quit".
>
> Robert Haas had an idea how to provide useful hints without breaking anything,
> but it seemed a little complicated.

It's not really that complicated.  Here's a patch.  This follows what
Tom suggested in http://postgr.es/m/30157.1513058...@sss.pgh.pa.us and
what I suggested in
https://www.postgresql.org/message-id/CA%2BTgmoZswp00PtcgPfQ9zbbh7HUTgsLLJ9Z1x9E2s8Y7ep048g%40mail.gmail.com

I've discovered one thing about this design that is not so good, which
is that if you open a single, double, or dollar quote, then the
instructions that are provided under that design do not work:

rhaas=# select $$
rhaas$# quit
Use \q to quit or press control-C to clear the input buffer.
rhaas$# \q
rhaas$# well this sucks
rhaas$#

Obviously this leaves something to be desired, but I think it's
probably just a matter of rephrasing the hint somehow.  I didn't have
a good idea off-hand though, so here's the patch as I have it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


quit-exit-rmh-v1.patch
Description: Binary data

Re: Rangejoin rebased

[Robert Haas]

On Tue, Jan 9, 2018 at 11:24 PM, Jeff Davis  wrote:
>> Just to emphasise why we want this, it might be better for the EXPLAIN
>> to say "Time Range Join" when the ranges being joined are Time Ranges,
>> and for other cases to just say "Range Join". The use of the word
>> Merge doesn't help much there.
>
> I don't care for special-casing the word "time" in there, because no
> other type would benefit. It seems a little too magical. I also do
> like leaving "merge" in there because it helps the user understand why
> their inputs are being sorted.

+1.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: proposal: alternative psql commands quit and exit

[Stephen Frost]

Robert,

* Robert Haas (robertmh...@gmail.com) wrote:
> On Mon, Jan 15, 2018 at 7:28 AM, Laurenz Albe  
> wrote:
> > Everaldo Canuto wrote:
> >> Also I don't see a consensus on this thread and I don't understand how 
> >> decisions are taken.
> >
> > It's just difficult to find consensus with many people.
> >
> > There were several valid concerns with this, the most striking (to me)
> > was Tome's concern that there are perfectly valid multi-line SQL statements
> > where a line could start with "exit" or "quit".
> >
> > Robert Haas had an idea how to provide useful hints without breaking 
> > anything,
> > but it seemed a little complicated.
> 
> It's not really that complicated.  Here's a patch.  This follows what
> Tom suggested in http://postgr.es/m/30157.1513058...@sss.pgh.pa.us and
> what I suggested in
> https://www.postgresql.org/message-id/CA%2BTgmoZswp00PtcgPfQ9zbbh7HUTgsLLJ9Z1x9E2s8Y7ep048g%40mail.gmail.com
> 
> I've discovered one thing about this design that is not so good, which
> is that if you open a single, double, or dollar quote, then the
> instructions that are provided under that design do not work:
> 
> rhaas=# select $$
> rhaas$# quit
> Use \q to quit or press control-C to clear the input buffer.
> rhaas$# \q
> rhaas$# well this sucks
> rhaas$#

> Obviously this leaves something to be desired, but I think it's
> probably just a matter of rephrasing the hint somehow.  I didn't have
> a good idea off-hand though, so here's the patch as I have it.

Well, control-C would still work in that case, so I'm not sure that it's
so bad.  It's certainly better than what we have now.  I would have
thought to include a hint to use \? for help too but it might make the
hint too long.  How about putting control-C first, like this?:

Press control-C to abort a command, \q to quit, or \? for help

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Robert Haas  writes:
> I've discovered one thing about this design that is not so good, which
> is that if you open a single, double, or dollar quote, then the
> instructions that are provided under that design do not work:

I was kind of imagining that we could make the hint text vary depending
on the parsing state.  Maybe that's too hard to get to --- but if the
prompt-creating code knows what it is, perhaps this can too.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Mon, Jan 15, 2018 at 10:48 AM, Stephen Frost  wrote:
> Well, control-C would still work in that case, so I'm not sure that it's
> so bad.  It's certainly better than what we have now.  I would have
> thought to include a hint to use \? for help too but it might make the
> hint too long.  How about putting control-C first, like this?:
>
> Press control-C to abort a command, \q to quit, or \? for help

One problem with this (and to some extent also with my version) is
that it doesn't really make it entirely clear that aborting a command
(or in the case of my version, clearing the input buffer) is something
you really ought to think about doing.  You might see that message and
not quite realize that there is a command in progress; if that weren't
an issue, we wouldn't need to print anything here at all.  It's almost
like we want to say something along the lines of "Hey there,
interactive user!  What you just typed looked suspiciously like a
command but, because there is text in the query buffer, I didn't treat
it as one and printed this ridiculously long blurb instead.  If you
want me to treat it as a command, press ^C to clear the query buffer
and then type that same thing again.  Thanks!"  However, that's a bit
long and, while there are some obvious ways to shorten it somewhat, I
can't really think of an overall phrasing that fits into an
80-character line without losing clarity.

The other point I would make is that, while I agree with you that the
patch is an improvement over the status quo, I'm not really keen to
print a message telling people to use \q or \? when in fact that those
things might not work.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: proposal: alternative psql commands quit and exit

[Robert Haas]

On Mon, Jan 15, 2018 at 10:57 AM, Tom Lane  wrote:
> Robert Haas  writes:
>> I've discovered one thing about this design that is not so good, which
>> is that if you open a single, double, or dollar quote, then the
>> instructions that are provided under that design do not work:
>
> I was kind of imagining that we could make the hint text vary depending
> on the parsing state.  Maybe that's too hard to get to --- but if the
> prompt-creating code knows what it is, perhaps this can too.

prompt_status does seem to be available in psql's MainLoop(), so I
think that could be done, but the problem is that I don't know exactly
what message would be useful to print when we're in a "in quotes"
state.  If I had a good message text for that state, I might just
choose to use it always rather than multiplying the number of
messages.

More broadly, I think what is needed here is less C-fu than
English-fu.  If we come up with something good, we can make it print
that thing.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: proposal: alternative psql commands quit and exit

[Geoff Winkless]

On 15 January 2018 at 16:10, Robert Haas  wrote:
>
> More broadly, I think what is needed here is less C-fu than
> English-fu.  If we come up with something good, we can make it print
> that thing.
>
Can we not just say "ctrl-D to quit" instead of \q? Doesn't that work
everywhere?

Geoff

Re: proposal: alternative psql commands quit and exit

[Laurenz Albe]

Geoff Winkless wrote:
> Can we not just say "ctrl-D to quit" instead of \q? Doesn't that work
> everywhere?

Not on Windows, as far as I know.

Yours,
Laurenz Albe

Re: proposal: alternative psql commands quit and exit

[Stephen Frost]

Geoff, all,

* Geoff Winkless (pgsqlad...@geoff.dj) wrote:
> On 15 January 2018 at 16:10, Robert Haas  wrote:
> >
> > More broadly, I think what is needed here is less C-fu than
> > English-fu.  If we come up with something good, we can make it print
> > that thing.
>
> Can we not just say "ctrl-D to quit" instead of \q? Doesn't that work
> everywhere?

My favorite part of this thread so far is actually realizing that when
someone hits ctrl-D, we'll happily print '\q' even if we were in quotes:

=> select $$
-> oh look
-> \q
-> doesn't work
-> but if I hit ctrl-D...
-> \q
➜  ~ 

Which is pretty cute considering \q doesn't actually work there. :)

Thanks!

Stephen


signature.asc
Description: PGP signature

Re: proposal: alternative psql commands quit and exit

[Geoff Winkless]

On 15 January 2018 at 16:16, Laurenz Albe  wrote:
> Geoff Winkless wrote:
>> Can we not just say "ctrl-D to quit" instead of \q? Doesn't that work
>> everywhere?
>
> Not on Windows, as far as I know.

Well on Windows, Ctrl-C doesn't clear the input buffer either - it
quits the whole thing.

Perhaps different messages on different OSes? :)

Others point out (elsewhere) that Ctrl-D fails if there's anything
before it on the individual line, which is news to me. Is that a
readline thing?

Geoff

Re: Implementing SQL ASSERTION

[David Fetter]

On Mon, Jan 15, 2018 at 03:40:57PM +0100, Fabien COELHO wrote:
> 
> >>I'm wondering about the effect of MVVC on this: if the check is
> >>performed when the INSERT is done, concurrent inserting transactions
> >>would count the current status which would be ok, but on commit all
> >>concurrent inserts would be there and the count could not be ok anymore?
> 
> >The patch doesn’t attempt to address concurrency (beyond the obvious
> >benefit of reducing the circumstances under which the assertion is
> >checked). I am working under the assumption that we will find some
> >acceptable way for that to be resolved :-) And at the moment, working in
> >serialisable mode addresses this issue. I think that is suggested in the
> >thread actually (essentially, if you want to use assertions, you require
> >that transactions be performed at serialisable isolation level).
> 
> Thanks for the pointers. The "serializable" isolation level restriction
> sounds reasonnable.

It sounds reasonable enough that I'd like to make a couple of Modest
Proposals™, to wit:

- We follow the SQL standard and make SERIALIZABLE the default
  transaction isolation level, and

- We disallow writes at isolation levels other than SERIALIZABLE when
  any ASSERTION could be in play.

That latter could range in implementation from crashingly unsubtle to
very precise.  

Crashingly Unsubtle:

Disallow writes at any isolation level other than SERIALIZABLE.

Very Precise:

Disallow writes at any other isolation level when the ASSERTION
could come into play using the same machinery that enforces the
ASSERTION in the first place.

What say?

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: [HACKERS] postgres_fdw bug in 9.6

[Robert Haas]

On Mon, Jan 15, 2018 at 3:09 AM, Etsuro Fujita
 wrote:
> Yeah, but I don't think the above example is good enough to explain that,
> because I think the bar/baz join would produce at most one tuple in an EPQ
> recheck since we would have only one EPQ tuple for both bar and baz in that
> recheck, and the join is inner.  I think such an example would probably be
> given e.g., by a modified version of the SQL where we have a full join of
> bar and baz, not an inner join.

Hmm, I was thinking that bar and baz wouldn't be constrained to return
just one tuple in that case, but I'm wrong: there would just be one
tuple per relation in that case.  However, that would also be true for
a full join, wouldn't it?

Regardless of that, the patch fixes the reported problem with very
little code change, and somebody can always improve it further later.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] [FEATURE PATCH] pg_stat_statements with plans (v02)

[Julian Markwort]

On 01/11/2018 03:43 PM, David Fetter wrote:

Is the assumption of a normal distribution reasonable for outlier
plans as you've seen them?

This is a difficult but fair question.
First of all, I'd like to clarify that the normal distribution is 
assumed for the set of all execution times matching a queryid; No 
assumptions are made about the distribution of the outliers themselves.
The primary goal of this approach was the limitation of plan updates, to 
avoid unnecessary IO operations.
When query performance does not vary much, no updates of the plans 
should be necessary, but as soon as query performance varies too much, 
the new plan should be stored.
For the purpose of distinguishing reasonable variance between execution 
times and great variance due to changing conditions which ultimately 
might result in a different plan, the assumption of a normal 
distribution for all execution times suits well.


Based on some early testing, this results in only a few percent of 
updates (1-3%) in relation to the total number of calls, when running 
some short pgbench tests.
As the sample size grows, the assumption of a normal distribution 
becomes increasingly accurate and the (unnecessary) sampling of plans 
decreases.
In a different test, I ran several queries with identical table sizes, 
the queries were fairly simple,  and the statistical evaluation led to 
few updates during these tests. When I increased the table size 
significantly, the database switched to a different plan. Because the 
execution time differed significantly, this new bad plan was stored. 
Similarly, after running a certain query a couple of times, I created an 
index on my test data, which resulted in a speedup which was significant 
enough to result in an update of the good plan.


Now, if a change to the data or the index situation only resulted in an 
insignificant performance increase or decrease (one that falls into the 
interval defined as [mean - 1.5*IQD, mean + 1-5*IQD] ), I think it might 
be possible to assume that we are not interested in an updated plan for 
this scenario.




smime.p7s
Description: S/MIME Cryptographic Signature

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Geoff Winkless  writes:
> Perhaps different messages on different OSes? :)

It's worse than that: the EOF key is configurable.  In principle we
could look into the tty settings and print the right thing, but
I doubt we want to go there, especially if there's no corresponding
thing on Windows.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Geoff Winkless]

On 15 January 2018 at 16:48, Tom Lane  wrote:
> Geoff Winkless  writes:
>> Perhaps different messages on different OSes? :)
>
> It's worse than that: the EOF key is configurable.  In principle we
> could look into the tty settings and print the right thing, but
> I doubt we want to go there, especially if there's no corresponding
> thing on Windows.

But surely if Windows always exits using Ctrl-C than that's easiest?

And while trying to find the EOF setting in libreadline might get
messy, you're already assuming that ctrl-C hasn't been knobbled using
stty intr. Unless you want to go searching for that too?

Geoff

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Robert Haas  writes:
> On Mon, Jan 15, 2018 at 10:57 AM, Tom Lane  wrote:
>> I was kind of imagining that we could make the hint text vary depending
>> on the parsing state.  Maybe that's too hard to get to --- but if the
>> prompt-creating code knows what it is, perhaps this can too.

> More broadly, I think what is needed here is less C-fu than
> English-fu.  If we come up with something good, we can make it print
> that thing.

Right, but if we're willing to look at the parse state, we don't need
to cram all possible knowledge into one 80-character string.  I'm
thinking about just responding to the current situation, say

You have begun a quoted string.  To end it, type '

(or ", or $foo$ as appropriate).  I'm inclined to violate our usual
message style guidelines here by not putting any quotes or punctuation
around the ending quote ... more likely to confuse than help.

Or, if you're not in a string but there's text in the buffer,

You have entered an incomplete SQL command.
Type ; to send it or \r to discard it.

Or, if there's no text in the buffer, we print the existing help
message for "help", or just exit for quit/exit.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[David Fetter]

On Mon, Jan 15, 2018 at 04:53:27PM +, Geoff Winkless wrote:
> On 15 January 2018 at 16:48, Tom Lane  wrote:
> > Geoff Winkless  writes:
> >> Perhaps different messages on different OSes? :)
> >
> > It's worse than that: the EOF key is configurable.  In principle
> > we could look into the tty settings and print the right thing, but
> > I doubt we want to go there, especially if there's no
> > corresponding thing on Windows.
> 
> But surely if Windows always exits using Ctrl-C than that's easiest?
> 
> And while trying to find the EOF setting in libreadline might get
> messy, you're already assuming that ctrl-C hasn't been knobbled
> using stty intr. Unless you want to go searching for that too?

I'm pretty sure changing either or both of those settings would
qualify as pilot error, and pretty abstruse pilot error at that.

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: proposal: alternative psql commands quit and exit

[Tom Lane]

Geoff Winkless  writes:
> And while trying to find the EOF setting in libreadline might get
> messy, you're already assuming that ctrl-C hasn't been knobbled using
> stty intr. Unless you want to go searching for that too?

Yeah, that's why I personally don't want to mention ctrl-C in this
message.  If we do mention it we're going to need to look up what
the signal is really bound to.

regards, tom lane

Re: proposal: alternative psql commands quit and exit

[Geoff Winkless]

On 15 January 2018 at 16:56, David Fetter  wrote:
> On Mon, Jan 15, 2018 at 04:53:27PM +, Geoff Winkless wrote:
>> And while trying to find the EOF setting in libreadline might get
>> messy, you're already assuming that ctrl-C hasn't been knobbled
>> using stty intr. Unless you want to go searching for that too?
>
> I'm pretty sure changing either or both of those settings would
> qualify as pilot error, and pretty abstruse pilot error at that.

Oh I agree. I was merely pointing out that the likelihood of one is no
more than the other.

In fact, I _have_ worked on systems (in the 90s, admittedly) where
intr was mapped to the "Delete" key. I've never (to my knowledge)
worked on a system where EOF was mapped to anything other than CTRL-D
in a command-line context, although I can see that emacs obsessives
might want to have configured their readline that way, which would
stomp all over ctrl-D.

It also occurs to me that if someone has just typed quit or
exit space at the start-of-line should not be a problem for
Ctrl-D, because the buffer is empty.

I still think it (Ctrl-D on *nix, Ctrl-C on windows) is the best of a
bad bunch, to be honest.

Geoff

On 15 January 2018 at 16:56, David Fetter  wrote:
> On Mon, Jan 15, 2018 at 04:53:27PM +, Geoff Winkless wrote:
>> On 15 January 2018 at 16:48, Tom Lane  wrote:
>> > Geoff Winkless  writes:
>> >> Perhaps different messages on different OSes? :)
>> >
>> > It's worse than that: the EOF key is configurable.  In principle
>> > we could look into the tty settings and print the right thing, but
>> > I doubt we want to go there, especially if there's no
>> > corresponding thing on Windows.
>>
>> But surely if Windows always exits using Ctrl-C than that's easiest?
>>
>> And while trying to find the EOF setting in libreadline might get
>> messy, you're already assuming that ctrl-C hasn't been knobbled
>> using stty intr. Unless you want to go searching for that too?
>
> I'm pretty sure changing either or both of those settings would
> qualify as pilot error, and pretty abstruse pilot error at that.
>
> 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: [HACKERS] Early locking option to parallel backup

[Robert Haas]

On Thu, Jan 11, 2018 at 9:02 AM, Stephen Frost  wrote:
> Parallel pg_dump is based on synchronized transactions though and we
> have a bunch of checks in ImportSnapshot() because a pg_dump parallel
> worker also can't really be quite the same as a normal backend.  Perhaps
> we could add on more restrictions in ImportSnapshot() to match the
> restrictions for parallel-mode workers?  If we think there's other users
> of SET TRANSACTION SNAPSHOT then we might need to extend that command
> for this case, but that seems relatively straight-forward.  I don't know
> how reasonable the idea of taking a normally-started backend and making
> it close enough to a parallel worker when a SET TRANSACTION SNAPSHOT
> PARALLEL (or whatever) happens to allow it to skip the lock fairness is
> though.

It seems pretty tricky to me.  I actually don't think this use case
has all that much in common with the parallel query case.  Both can be
addressed by tweaking the lock manager, but I think this needs a
different set of tweaks than that did.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: pgbench - add \if support

[Fabien COELHO]

Here is a rebase. I made some tests use actual expressions instead of just 
0 and 1. No other changes.


Sigh. Better with the attachment. Sorry for the noise.


Here is a very minor rebase.

--
Fabien.diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index 3dd492c..c203c41 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -895,6 +895,21 @@ pgbench  options  d
   
 
   
+   
+\if expression
+\elif expression
+\else
+\endif
+
+  
+  This group of commands implements nestable conditional blocks,
+  similarly to psql's .
+  Conditional expressions are identical to those with \set,
+  with non-zero values interpreted as true.
+ 
+
+   
+

 
  \set varname expression
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index fce7e3a..e88f782 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -2148,7 +2148,7 @@ hello 10
   
 
 
-  
+  
 \if expression
 \elif expression
 \else
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 31ea6ca..b5ebefc 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -32,6 +32,7 @@
 #endif			/* ! WIN32 */
 
 #include "postgres_fe.h"
+#include "fe_utils/conditional.h"
 
 #include "getopt_long.h"
 #include "libpq-fe.h"
@@ -274,6 +275,9 @@ typedef enum
 	 * and we enter the CSTATE_SLEEP state to wait for it to expire. Other
 	 * meta-commands are executed immediately.
 	 *
+	 * CSTATE_SKIP_COMMAND for conditional branches which are not executed,
+	 * quickly skip commands that do not need any evaluation.
+	 *
 	 * CSTATE_WAIT_RESULT waits until we get a result set back from the server
 	 * for the current command.
 	 *
@@ -283,6 +287,7 @@ typedef enum
 	 * command counter, and loops back to CSTATE_START_COMMAND state.
 	 */
 	CSTATE_START_COMMAND,
+	CSTATE_SKIP_COMMAND,
 	CSTATE_WAIT_RESULT,
 	CSTATE_SLEEP,
 	CSTATE_END_COMMAND,
@@ -312,6 +317,7 @@ typedef struct
 	PGconn	   *con;			/* connection handle to DB */
 	int			id;/* client No. */
 	ConnectionStateEnum state;	/* state machine's current state. */
+	ConditionalStack cstack;	/* enclosing conditionals state */
 
 	int			use_file;		/* index in sql_script for this client */
 	int			command;		/* command number in script */
@@ -400,7 +406,11 @@ typedef enum MetaCommand
 	META_SET,	/* \set */
 	META_SETSHELL,/* \setshell */
 	META_SHELL,	/* \shell */
-	META_SLEEP	/* \sleep */
+	META_SLEEP,	/* \sleep */
+	META_IF,	/* \if */
+	META_ELIF,	/* \elif */
+	META_ELSE,	/* \else */
+	META_ENDIF	/* \endif */
 } MetaCommand;
 
 typedef enum QueryMode
@@ -1587,6 +1597,7 @@ setBoolValue(PgBenchValue *pv, bool bval)
 	pv->type = PGBT_BOOLEAN;
 	pv->u.bval = bval;
 }
+
 /* assign an integer value */
 static void
 setIntValue(PgBenchValue *pv, int64 ival)
@@ -2295,6 +2306,14 @@ getMetaCommand(const char *cmd)
 		mc = META_SHELL;
 	else if (pg_strcasecmp(cmd, "sleep") == 0)
 		mc = META_SLEEP;
+	else if (pg_strcasecmp(cmd, "if") == 0)
+		mc = META_IF;
+	else if (pg_strcasecmp(cmd, "elif") == 0)
+		mc = META_ELIF;
+	else if (pg_strcasecmp(cmd, "else") == 0)
+		mc = META_ELSE;
+	else if (pg_strcasecmp(cmd, "endif") == 0)
+		mc = META_ENDIF;
 	else
 		mc = META_NONE;
 	return mc;
@@ -2416,11 +2435,11 @@ preparedStatementName(char *buffer, int file, int state)
 }
 
 static void
-commandFailed(CState *st, const char *message)
+commandFailed(CState *st, const char *cmd, const char *message)
 {
 	fprintf(stderr,
-			"client %d aborted in command %d of script %d; %s\n",
-			st->id, st->command, st->use_file, message);
+			"client %d aborted in command %d (%s) of script %d; %s\n",
+			st->id, st->command, cmd, st->use_file, message);
 }
 
 /* return a script number with a weighted choice. */
@@ -2608,6 +2627,8 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 	st->state = CSTATE_START_THROTTLE;
 else
 	st->state = CSTATE_START_TX;
+/* check consistency */
+Assert(conditional_stack_empty(st->cstack));
 break;
 
 /*
@@ -2773,7 +2794,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 {
 	if (!sendCommand(st, command))
 	{
-		commandFailed(st, "SQL command send failed");
+		commandFailed(st, "SQL", "SQL command send failed");
 		st->state = CSTATE_ABORTED;
 	}
 	else
@@ -2806,7 +2827,7 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 
 		if (!evaluateSleep(st, argc, argv, &usec))
 		{
-			commandFailed(st, "execution of meta-command 'sleep' failed");
+			commandFailed(st, "sleep", "execution of meta-command failed");
 			st->state = CSTATE_ABORTED;
 			break;
 		}
@@ -2817,77 +2838,209 @@ doCustom(TState *thread, CState *st, StatsData *agg)
 		st->state = CSTATE_SLEEP;
 		break;
 	}
-	else
+	else if (command->meta == META_SET ||
+			 command-

Re: [HACKERS] postgres_fdw bug in 9.6

[Tom Lane]

Robert Haas  writes:
> I started to look at this patch again today and got cold feet.  It
> seems to me that this entire design on the posted patch is based on
> your remarks in http://postgr.es/m/13242.1481582...@sss.pgh.pa.us --

> # One way that we could make things better is to rely on the knowledge
> # that EPQ isn't asked to evaluate joins for more than one row per input
> # relation, and therefore using merge or hash join technology is really
> # overkill.  We could make a tree of simple nestloop joins, which aren't
> # going to care about sort order, if we could identify the correct join
> # clauses to apply.

> However, those remarks are almost entirely wrong.

Hm, one of us is confused.  Possibly it's me, but:

> It's true that the
> relations for which we have EPQ tuples will only ever output a single
> tuple,

Yup, and that is all of them.  We should have a tuple identity
(ctid or whole-row) for every base relation in an EPQ test.

> but because of what you said in the quoted text above, this
> does NOT imply that the recheck plan never needs to worry about
> dealing with a large number of rows, as the following example shows.

This example doesn't prove anything whatsoever about what happens during
EPQ, though, or at least it shouldn't.  If we're examining more than one
row then we're doing it wrong, because the point of the EPQ evaluation
is only to determine whether one single joined row (still) meets the
query conditions.

> After some thought, it seems that there's a much simpler way that we
> could fix the problem you identified in that original email: if the
> EPQ path isn't properly sorted, have postgres_fdw's
> add_paths_with_pathkeys_for_rel stick a Sort node on top of it.  I
> tried this and it does indeed fix Jeff Janes' initial test case.

Hm.  Simple is certainly good, but if there's multiple rows coming
back during an EPQ recheck then I think we have a performance problem.

regards, tom lane

Re: [HACKERS] proposal: psql command \graw

[Alexander Korotkov]

Hi!

On Mon, Dec 4, 2017 at 6:42 PM, Pavel Stehule 
wrote:

> 2017-12-04 9:29 GMT+01:00 Alexander Korotkov :
>
>> On Mon, Dec 4, 2017 at 11:21 AM, Alexander Korotkov <
>> a.korot...@postgrespro.ru> wrote:
>>
>>> The problem is that it's hard to read arbitrary formatted psql output
>>> from external program (not just gnuplot, but even especially written
>>> script).  I made my scripts read few variations, but it doesn't look
>>> feasible to read all the combinations.  For sure, it's possible to set
>>> format options inside macro, but then it would affect psql format options
>>> after execution.
>>>
>>> This is why I think only one \graw option is just fine, because it
>>> produces stable machine-readable output.
>>>
>>
>> Oh, I just get that in current state of \graw doesn't produce good
>> machine-readable output.
>>
>> # select '|', '|' \graw
>> |||
>>
>> Column separator is character which can occur in values, and values
>> aren't escaped.  Thus, reader can't correctly divide values between columns
>> in all the cases.  So, I would rather like to see \graw to output in csv
>> format with proper escaping.
>>
>
> current \graw implementation is pretty minimalistic
>
> It is interesting topic - the client side csv support.
>
> It can simplify lot of things
>

So, I see there is no arguing yet that exporting dataset from psql into a
pipe in machine-readable format (presumably csv) would be an useful feature.
Are you going to revise your patch that way during this commitfest?
I'm marking this patch as "waiting for author" for now.

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

Re: proposal: alternative psql commands quit and exit

[Geoff Winkless]

On 15 January 2018 at 17:03, Tom Lane  wrote:
> Geoff Winkless  writes:
>> And while trying to find the EOF setting in libreadline might get
>> messy, you're already assuming that ctrl-C hasn't been knobbled using
>> stty intr. Unless you want to go searching for that too?
>
> Yeah, that's why I personally don't want to mention ctrl-C in this
> message.  If we do mention it we're going to need to look up what
> the signal is really bound to.

Could always rewrite gets_interactive to implement the documented
"alternative interface" to readline

https://tiswww.cwru.edu/php/chet/readline/readline.html#SEC43

and force exit that way by intercepting Ctrl-D at the character level.
That does feel a bit (!) wrong though, and I'm not sure how difficult
it gets with different locales etc. Would also have to modify
gets_fromFile, in case readline was disabled.

At this point it depends quite how far down the rabbit-hole you want
to go to stop people googling "how do I exit psql", I suppose :p

Geoff

polymorphic parameters limits - correct solution?

[Pavel Stehule]

Hi

I played with introduction of new pair of Polymorphic Parameters - like
anyXelement and anyXarray. Now, I don't think so enhancing PP is good way
now. Without significant redesign there are not practical append more code
there.

Why this is a issue? The extension's authors are not able to specify result
type without enumeration of all possible function signatures. Similar
situation is in argument processing - there are workaround based on "any"
type.

Can we design helper function used only for function that returns "any"
type, that returns correct result type?

some like

CREATE OR REPLACE FUNCTION fx("any", "any")
RETURNS "any" AS ...

CREATE OR REPLACE FUNCTION fx_helper(Oid[])
RETURNS Oid AS ..

ALTER FUNCTION fx("any", "any") SET ('result_helper', 'fx_helper'::regproc);

possibly, there can be some argument helper, that can specify UNKNOWN
arguments.

Notes, comments?

Regards

Pavel

Re: proposal: alternative psql commands quit and exit

[Tom Lane]

Geoff Winkless  writes:
> At this point it depends quite how far down the rabbit-hole you want
> to go to stop people googling "how do I exit psql", I suppose :p

Well, I concur with Robert's comment upthread that we don't want to
print any advice that's possibly wrong.  So I'd rather provide hints
that are sure to work, rather than hints that might represent the
way to get out with fewest keystrokes, but could go wrong in unusual
situations.

regards, tom lane

Re: [HACKERS] Transaction control in procedures

[Andrew Dunstan]

On 01/05/2018 04:30 PM, Peter Eisentraut wrote:
> A merge conflict has arisen, so for simplicity, here is an updated patch.
>
> On 12/20/17 10:08, Peter Eisentraut wrote:
>> Updated patch attached.
>>
>> I have addressed the most recent review comments I believe.
>>
>> The question about what happens to cursor loops in PL/Perl and PL/Python
>> would be addressed by the separate thread "portal pinning".  The test
>> cases in this patch are currently marked by FIXMEs.
>>
>> I have changed the SPI API a bit.  I got rid of SPI_set_nonatomic() and
>> instead introduced SPI_connect_ext() that you can pass flags to.  The
>> advantage of that is that in the normal case we can continue to use the
>> existing memory contexts, so nothing changes for existing uses, which
>> seems desirable.  (This also appears to address some sporadic test
>> failures in PL/Perl.)
>>
>> I have also cleaned up the changes in portalmem.c further, so the
>> changes are now even smaller.
>>
>> The commit message in this patch contains more details about some of
>> these changes.


Generally looks good.

This confused me slightly:

+    Transactions cannot be ended inside loops through query results
or inside
+    blocks with exception handlers.

I suggest: "A transaction cannot be ended inside a loop over query
results, nor inside a block with exception handlers."

The patch has bitrotted slightly in src/backend/commands/portalcmds.c

The plperl expected file needs updating. Also, why does spi_commit() in
a loop result in an error message but not spi_rollback()?


cheers

andrew



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

Re: [HACKERS] proposal: psql command \graw

[Pavel Stehule]

2018-01-15 18:40 GMT+01:00 Alexander Korotkov :

> Hi!
>
> On Mon, Dec 4, 2017 at 6:42 PM, Pavel Stehule 
> wrote:
>
>> 2017-12-04 9:29 GMT+01:00 Alexander Korotkov :
>>
>>> On Mon, Dec 4, 2017 at 11:21 AM, Alexander Korotkov <
>>> a.korot...@postgrespro.ru> wrote:
>>>
 The problem is that it's hard to read arbitrary formatted psql output
 from external program (not just gnuplot, but even especially written
 script).  I made my scripts read few variations, but it doesn't look
 feasible to read all the combinations.  For sure, it's possible to set
 format options inside macro, but then it would affect psql format options
 after execution.

 This is why I think only one \graw option is just fine, because it
 produces stable machine-readable output.

>>>
>>> Oh, I just get that in current state of \graw doesn't produce good
>>> machine-readable output.
>>>
>>> # select '|', '|' \graw
>>> |||
>>>
>>> Column separator is character which can occur in values, and values
>>> aren't escaped.  Thus, reader can't correctly divide values between columns
>>> in all the cases.  So, I would rather like to see \graw to output in csv
>>> format with proper escaping.
>>>
>>
>> current \graw implementation is pretty minimalistic
>>
>> It is interesting topic - the client side csv support.
>>
>> It can simplify lot of things
>>
>
> So, I see there is no arguing yet that exporting dataset from psql into a
> pipe in machine-readable format (presumably csv) would be an useful feature.
> Are you going to revise your patch that way during this commitfest?
> I'm marking this patch as "waiting for author" for now.
>

I hope so lot of people invite CSV support on client side. Some like:

psql -c "SELECT * FROM pg_class" --csv --header > pg_class.csv

Client side CSV formatting is significantly better idea. Unfortunately
there are not clean how to do it. The basic question is: can we have 2
codes for CSV - server side/client side? If yes, then implementation should
be trivial. If not, then I have not any idea.

We are able to generate html/tex/markdown formats on client side. CSV is
more primitive, but much more important data format, so it should not be a
problem. But I remember some objections related to code duplication.

Regards

Pavel


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

Re: proposal: alternative psql commands quit and exit

[Chapman Flack]

On 01/15/18 11:56, David Fetter wrote:
>> And while trying to find the EOF setting in libreadline might get
>> messy, you're already assuming that ctrl-C hasn't been knobbled
>> using stty intr. Unless you want to go searching for that too?
> 
> I'm pretty sure changing either or both of those settings would
> qualify as pilot error, and pretty abstruse pilot error at that.

I once worked in a shop where the EOF default had in fact been changed
to ^Z on the Unix boxen, for consistency with Windows. I may have had
a personal opinion about it not unlike yours, but the pilot at the time
being my boss, I just made it my business to cheerfully explain it every.
time. it. needed. explaining.

So, it's a thing that can happen.

-Chap

Re: New gist vacuum.

[Alexander Korotkov]

Hi!

On Sat, Dec 30, 2017 at 12:18 PM, Andrey Borodin 
wrote:

> > 28 дек. 2017 г., в 16:37, Andrey Borodin 
> написал(а):
> > Here is new version of the patch for GiST VACUUM.
> > There are two main changes:
> > 1. During rescan for page deletion only know to be recently empty pages
> are rescanned.
> > 2. I've re-implemented physical scan with array instead of hash table.
>
> There is one more minor spot in GiST VACUUM. It takes heap tuples count
> for statistics for partial indexes, while it should not.
>
> If gistvacuumcleanup() is not given a statistics gathered by
> gistbulkdelete() it returns incorrect tuples count for partial index.
> Here's the micropatch, which fixes that corner case.
> To reproduce this effect I used this query:
> create table y as select cube(random()) c from generate_series(1,1) y;
> create index on y using gist(c) where c~>1 > 0.5;
> vacuum verbose y;
> Before patch it will report 1 tuples, with patch it will report
> different values around 5000.
>

It's very good that you've fixed that.

I do not know, should I register separate commitfest entry? The code is
> very close to main GiST VACUUM patch, but solves a bit different problem.
>

Yes, I think it deserves separate commitfest entry.  Despite it's related
to GiST VACUUM, it's a separate fix.
I've made small improvements to this patch: variable naming, formatting,
comments.
BTW, do we really need to set shouldCount depending on whether we receive
stats argument or not?  What if we always set shouldCount as in the first
branch of "if"?


> shouldCount = !heap_attisnull(rel->rd_indextuple, Anum_pg_index_indpred) ||
>info->estimated_count;



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


0001-Count-tuples-correctly-during-GiST-VACUUM-of-partial-2.patch
Description: Binary data

Re: New gist vacuum.

[Alexander Korotkov]

Hi!

On Thu, Dec 28, 2017 at 2:37 PM, Andrey Borodin 
wrote:

> > 19 дек. 2017 г., в 15:58, Andrey Borodin 
> написал(а):
> > Here is the patch that deletes pages during GiST VACUUM.
>
> Here is new version of the patch for GiST VACUUM.
> There are two main changes:
> 1. During rescan for page deletion only know to be recently empty pages
> are rescanned.
> 2. I've re-implemented physical scan with array instead of hash table.
>

I'm very glad this patch isn't forgotten.  I've assigned to review this
patch.
My first note on this patchset is following.  These patches touches
sensitive aspects or GiST and are related to complex concurrency issues.
Thus, I'm sure both of them deserve high-level description in
src/backend/access/gist/README.  Given this description, it would be much
easier to review the patches.

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

Re: [PROPOSAL] Shared Ispell dictionaries

[Arthur Zakirov]

On Sat, Jan 13, 2018 at 10:33:14PM +0100, Tomas Vondra wrote:
> Not sure if we really need to add the database/schema OIDs. I mentioned
> the unexpected consequences (cross-db sharing) but maybe that's a
> feature we should keep (it reduces memory usage). So perhaps this should
> be another CREATE TEXT SEARCH DICTIONARY parameter, allowing sharing the
> dictionary with other databases?
> 
> Aren't we overengineering this?

Another related problem I've noticed is memory leak. When a dictionary
loaded and then dropped it won't be unloaded. I see several approaches:

1 - Use Oid of the dictionary itself as the key instead dictfile and
afffile. When the dictionary is dropped it will be easily unloaded if it
was loaded. Implementing should be easy, but the drawback is more memory 
consumption.
2 - Use reference counter with cross-db sharing. When the dictionary is
loaded the counter increases. If all record of loaded dictionary is dropped
it will be unloaded.
3 - Or reference counters without cross-db sharing to avoid possible confusing.
Here dictfile, afffile and database Oid will be used as the key.

-- 
Arthur Zakirov
Postgres Professional: http://www.postgrespro.com
Russian Postgres Company

Re: [HACKERS] postgres_fdw bug in 9.6

[Robert Haas]

On Mon, Jan 15, 2018 at 12:31 PM, Tom Lane  wrote:

>> After some thought, it seems that there's a much simpler way that we
>> could fix the problem you identified in that original email: if the
>> EPQ path isn't properly sorted, have postgres_fdw's
>> add_paths_with_pathkeys_for_rel stick a Sort node on top of it.  I
>> tried this and it does indeed fix Jeff Janes' initial test case.
>
> Hm.  Simple is certainly good, but if there's multiple rows coming
> back during an EPQ recheck then I think we have a performance problem.

You are correct ... I was wrong about that part, and said so in an
email on this thread sent about 45 minutes before yours.  However, I
still think the patch is a good fix for the immediate issue, unless
you see some problem with it.  It's simple and back-patchable and does
not preclude further work anybody, including you, might want to do in
the future.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: proposal: alternative psql commands quit and exit

[Tels]

On Mon, January 15, 2018 11:10 am, Robert Haas wrote:
> On Mon, Jan 15, 2018 at 10:57 AM, Tom Lane  wrote:
>> Robert Haas  writes:
>>> I've discovered one thing about this design that is not so good, which
>>> is that if you open a single, double, or dollar quote, then the
>>> instructions that are provided under that design do not work:
>>
>> I was kind of imagining that we could make the hint text vary depending
>> on the parsing state.  Maybe that's too hard to get to --- but if the
>> prompt-creating code knows what it is, perhaps this can too.
>
> prompt_status does seem to be available in psql's MainLoop(), so I
> think that could be done, but the problem is that I don't know exactly
> what message would be useful to print when we're in a "in quotes"
> state.  If I had a good message text for that state, I might just
> choose to use it always rather than multiplying the number of
> messages.
>
> More broadly, I think what is needed here is less C-fu than
> English-fu.  If we come up with something good, we can make it print
> that thing.

Maybe that's a Linux-specific thing, but I always use Ctrl-D to exit a
console, and this works with psql, too, even when in the middle of a query
typed.

So maybe this could be suggested?

Best wishes,

Tels

Re: proposal: alternative psql commands quit and exit

[Tels]

Moin,

On Mon, January 15, 2018 2:34 pm, Tels wrote:

> Maybe that's a Linux-specific thing, but I always use Ctrl-D to exit a
> console, and this works with psql, too, even when in the middle of a query
> typed.
>
> So maybe this could be suggested?

Sorry, should have really read the thread until the very end, please
ignore my noise.

Best wishes,

Tels

Re: [HACKERS] Parallel tuplesort (for parallel B-Tree index creation)

[Peter Geoghegan]

On Sun, Jan 14, 2018 at 8:25 PM, Amit Kapila  wrote:
> On Sun, Jan 14, 2018 at 1:43 AM, Peter Geoghegan  wrote:
>> On Sat, Jan 13, 2018 at 4:32 AM, Amit Kapila  wrote:
>>> Yeah, but this would mean that now with parallel create index, it is
>>> possible that some tuples from the transaction would end up in index
>>> and others won't.
>>
>> You mean some tuples from some past transaction that deleted a bunch
>> of tuples and committed, but not before someone acquired a still-held
>> snapshot that didn't see the deleter's transaction as committed yet?
>>
>
> I think I am talking about something different.  Let me try to explain
> in some more detail.  Consider a transaction T-1 has deleted two
> tuples from tab-1, first on page-1 and second on page-2 and committed.
> There is a parallel transaction T-2 which has an open snapshot/query
> due to which oldestXmin will be smaller than T-1.   Now, in another
> session, we started parallel Create Index on tab-1 which has launched
> one worker.  The worker decided to scan page-1 and will found that the
> deleted tuple on page-1 is Recently Dead, so will include it in Index.
> In the meantime transaction, T-2 got committed/aborted which allows
> oldestXmin to be greater than the value of transaction T-1 and now
> leader decides to scan the page-2 with freshly computed oldestXmin and
> found that the tuple on that page is Dead and has decided not to
> include it in the index.  So, this leads to a situation where some
> tuples deleted by the transaction will end up in index whereas others
> won't.  Note that I am not arguing that there is any fundamental
> problem with this, but just want to highlight that such a case doesn't
> seem to exist with Create Index.

I must have not done a good job of explaining myself ("You mean some
tuples from some past transaction..."), because this is exactly what I
meant, and was exactly how I understood your original remarks from
Saturday.

In summary, while I do agree that this is different to what we see
with serial index builds, I still don't think that this is a concern
for us.

-- 
Peter Geoghegan

Re: Implementing SQL ASSERTION

[Joe Wildish]

Hi David,

> On 15 Jan 2018, at 16:35, David Fetter  wrote:
> 
> It sounds reasonable enough that I'd like to make a couple of Modest
> Proposals™, to wit:
> 
> - We follow the SQL standard and make SERIALIZABLE the default
>  transaction isolation level, and
> 
> - We disallow writes at isolation levels other than SERIALIZABLE when
>  any ASSERTION could be in play.

Certainly it would be easy to put a test into the assertion check function to 
require the isolation level be serialisable. I didn’t realise that that was 
also the default level as per the standard. That need not necessarily be 
changed, of course; it would be obvious to the user that it was a requirement 
as the creation of an assertion would fail without it, as would any subsequent 
attempts to modify the involved tables.

-Joe

Re: [HACKERS] WIP: Aggregation push-down

[Robert Haas]

On Fri, Dec 22, 2017 at 10:43 AM, Antonin Houska  wrote:
> Michael Paquier  wrote:
>> On Sat, Nov 4, 2017 at 12:33 AM, Antonin Houska  wrote:
>> > I'm not about to add any other features now. Implementation of the missing
>> > parts (see the TODO comments in the code) is the next step. But what I'd
>> > appreciate most is a feedback on the design. Thanks.
>>
>> I am getting a conflict after applying patch 5 but this did not get
>> any reviews so moved to next CF with waiting on author as status.
>
> Attached is the next version.

I've been a bit confused for a while about what this patch is trying
to do, so I spent some time today looking at it to try to figure it
out.  There's a lot I don't understand yet, but it seems like the
general idea is to build, potentially for each relation in the join
tree, not only the regular list of paths but also a list of "grouped"
paths.  If the pre-grouped path wins, then we can get a final path
that looks like Finalize Aggregate -> Some Joins -> Partial Aggregate
-> Maybe Some More Joins -> Base Table Scan.  In some cases the patch
seems to replace that uppermost Finalize Aggregate with a Result node.

translate_expression_to_rels() looks unsafe.  Equivalence members are
known to be equal under the semantics of the relevant operator class,
but that doesn't mean that one can be freely substituted for another.
For example:

rhaas=# create table one (a numeric);
CREATE TABLE
rhaas=# create table two (a numeric);
CREATE TABLE
rhaas=# insert into one values ('0');
INSERT 0 1
rhaas=# insert into two values ('0.00');
INSERT 0 1
rhaas=# select one.a, count(*) from one, two where one.a = two.a group by 1;
 a | count
---+---
 0 | 1
(1 row)

rhaas=# select two.a, count(*) from one, two where one.a = two.a group by 1;
  a   | count
--+---
 0.00 | 1
(1 row)

There are, admittedly, a large number of data types for which such a
substitution would work just fine.  numeric will not, citext will not,
but many others are fine. Regrettably, we have no framework in the
system for identifying equality operators which actually test identity
versus some looser notion of equality.  Cross-type operators are a
problem, too; if we have foo.x = bar.y in the query, one might be int4
and the other int8, for example, but they can still belong to the same
equivalence class.

Concretely, in your test query "SELECT p.i, avg(c1.v) FROM
agg_pushdown_parent AS p JOIN agg_pushdown_child1 AS c1 ON c1.parent =
p.i GROUP BY p.i" you assume that it's OK to do a Partial
HashAggregate over c1.parent rather than p.i.  This will be false if,
say, c1.parent is of type citext and p.i is of type text; this will
get grouped together that shouldn't.  It will also be false if the
grouping expression is something like GROUP BY length(p.i::text),
because one value could be '0'::numeric and the other '0.00'::numeric.
I can't think of a reason why it would be false if the grouping
expressions are both simple Vars of the same underlying data type, but
I'm a little nervous that I might be wrong even about that case.
Maybe you've handled all of this somehow, but it's not obvious to me
that it has been considered.

Another thing I noticed is that the GroupedPathInfo looks a bit like a
stripped-down RelOptInfo, in that for example it has both a pathlist
and a partial_pathlist. I'm inclined to think that we should build new
RelOptInfos instead.  As you have it, there are an awful lot of things
that seem to know about the grouped/ungrouped distinction, many of
which are quite low-level functions like add_path(),
add_paths_to_append_rel(), try_nestloop_path(), etc.  I think that
some of this would go away if you had separate RelOptInfos instead of
GroupedPathInfo.

Some compiler noise:

allpaths.c:2794:11: error: variable 'partial_target' is used
uninitialized whenever 'if' condition is false
  [-Werror,-Wsometimes-uninitialized]
else if (rel->gpi != NULL && rel->gpi->target != NULL)
 ^~~~
allpaths.c:2798:56: note: uninitialized use occurs here
create_gather_path(root, rel, cheapest_partial_path,
partial_target,

^~
allpaths.c:2794:7: note: remove the 'if' if its condition is always true
else if (rel->gpi != NULL && rel->gpi->target != NULL)
 ^
allpaths.c:2794:11: error: variable 'partial_target' is used
uninitialized whenever '&&' condition is false
  [-Werror,-Wsometimes-uninitialized]
else if (rel->gpi != NULL && rel->gpi->target != NULL)
 ^~~~
allpaths.c:2798:56: note: uninitialized use occurs here
create_gather_path(root, rel, cheapest_partial_path,
partial_target,

^~
allpaths.c:2794:11: note: remove the '&&' if its condition is always true
else if (rel->gpi != NULL && rel->gpi->target != NULL)
 ^~~
allpaths.c:2773:28: note: initialize the va

Re: proposal: alternative psql commands quit and exit

[David Fetter]

On Mon, Jan 15, 2018 at 01:24:45PM -0500, Chapman Flack wrote:
> On 01/15/18 11:56, David Fetter wrote:
> >> And while trying to find the EOF setting in libreadline might get
> >> messy, you're already assuming that ctrl-C hasn't been knobbled
> >> using stty intr. Unless you want to go searching for that too?
> > 
> > I'm pretty sure changing either or both of those settings would
> > qualify as pilot error, and pretty abstruse pilot error at that.
> 
> I once worked in a shop where the EOF default had in fact been changed
> to ^Z on the Unix boxen, for consistency with Windows. I may have had
> a personal opinion about it not unlike yours, but the pilot at the time
> being my boss, I just made it my business to cheerfully explain it every.
> time. it. needed. explaining.
> 
> So, it's a thing that can happen.

Of course it is. It's just not a thing I believe we should perturb
our code or documentation to accommodate.

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: [HACKERS] postgres_fdw bug in 9.6

[Tom Lane]

Robert Haas  writes:
> On Mon, Jan 15, 2018 at 12:31 PM, Tom Lane  wrote:
>> Hm.  Simple is certainly good, but if there's multiple rows coming
>> back during an EPQ recheck then I think we have a performance problem.

> You are correct ... I was wrong about that part, and said so in an
> email on this thread sent about 45 minutes before yours.  However, I
> still think the patch is a good fix for the immediate issue, unless
> you see some problem with it.  It's simple and back-patchable and does
> not preclude further work anybody, including you, might want to do in
> the future.

I spent some more time poking at this.  I confirmed that there is *not*
a performance problem: as I'd thought, during an EPQ recheck each base
scan relation will return its single EPQ tuple, without any communication
with the remote server, whether or not the rel was marked FOR UPDATE.

There *is* a problem with GetExistingLocalJoinPath not honoring its
API spec: it will sometimes return join nests that include remote
joins at levels below the top, as I'd speculated to begin with.
This turns out not to be a problem for postgres_fdw, because any
such remote-join node will just recursively fob off EPQ checks
onto its own outerpath, so that eventually you get down to base
scan relations anyway, and everything works.  However, because
GetExistingLocalJoinPath is claimed to be a general purpose function
useful for other FDWs, the fact that its API contract is a lie
seems to me to be a problem anyway.  Another FDW might have a stronger
dependency on there not being remote sub-joins.  (Reviewing the thread
history, I see that Ashutosh agreed this was possible at

and suggested that we just change the function's commentary to not
make promises it won't keep.  Maybe that's enough, but I think just
walking away isn't enough.)

I'm also still pretty unhappy with the amount of useless planning work
caused by doing GetExistingLocalJoinPath during path creation.  It strikes
me that we could likely replace the entire thing with some code that just
reconstructs the join node's output tuple during EPQ using the rowmark
data for all the base relations.  Outer joins aren't really a problem:
we could tell which relations were replaced by nulls because the rowmark
values that bubbled up to the top went to nulls themselves.  However,
that's a nontrivial amount of work and probably wouldn't result in
something we cared to back-patch, especially since it's not really a bug
fix.

Anyway, I agree with you that we know of no observable bug here except
for Jeff's original complaint, and forcing sorting of the EPQ paths
would be enough to fix that.

Your patch seems OK codewise, but I think the comment is not nearly
adequate.  Maybe something like "The EPQ path must be at least as well
sorted as the path itself, in case it gets used as input to a mergejoin."

Also, a regression test case would be appropriate perhaps.  I tried
to reproduce Jeff's complaint in the postgres_fdw regression database,
and it actually failed on the very first multi-way join I tried:

contrib_regression=# explain select * from ft1,ft2,ft4,ft5 where ft1.c1=ft2.c1 
and ft1.c1=ft4.c1 and ft1.c1=ft5.c1 for update;
ERROR:  outer pathkeys do not match mergeclauses

Also, for the archives' sake, here's an example showing that
GetExistingLocalJoinPath is not doing what it says on the tin:

contrib_regression=# set from_collapse_limit TO 1;
SET
contrib_regression=# set enable_hashjoin TO 0;
SET
contrib_regression=# set enable_mergejoin TO 0;
SET
contrib_regression=# explain select ft1.c1,ft4.* from ft1,ft2 left join ft4 on 
ft2.c1=ft4.c1,ft5,loc1 where ft1.c1=ft2.c1 and ft1.c1=ft5.c1 and ft1.c1=loc1.f1 
for update of ft1,loc1;
   QUERY PLAN   
 
-
 LockRows  (cost=105.72..781.42 rows=210 width=286)
   ->  Nested Loop  (cost=105.72..779.32 rows=210 width=286)
 Join Filter: (ft2.c1 = loc1.f1)
 ->  Seq Scan on loc1  (cost=0.00..22.70 rows=1270 width=10)
 ->  Materialize  (cost=105.72..128.06 rows=33 width=179)
   ->  Foreign Scan  (cost=105.72..127.89 rows=33 width=179)
 Relations: ((public.ft1) INNER JOIN ((public.ft2) LEFT 
JOIN (public.ft4))) INNER JOIN (public.ft5)
 ->  Nested Loop  (cost=233.62..721.53 rows=136 width=179)
   Join Filter: (ft2.c1 = ft5.c1)
   ->  Nested Loop  (cost=202.12..12631.43 rows=822 
width=137)
 Join Filter: (ft1.c1 = ft2.c1)
 ->  Foreign Scan on ft1  (cost=100.00..141.00 
rows=1000 width=75)
 ->  Materialize  (cost=102.12..162.48 rows=822 
width=83)
   ->  Foreign Scan  (cost=102.12..158.37 
r

Re: [HACKERS] Partition-wise aggregation/grouping

[Robert Haas]

On Thu, Jan 11, 2018 at 6:00 AM, Jeevan Chalke
 wrote:
>  Attached new set of patches adding this. Only patch 0007 (main patch) and
> 0008 (testcase patch) has changed.
>
> Please have a look and let me know if I missed any.

I spent a little time studying 0001 and 0002 today, as well as their
relation with 0007.  I find the way that the refactoring has been done
slightly odd.  With 0001 and 0002 applied, we end up with three
functions for creating aggregate paths: create_partial_agg_path, which
handles the partial-path case for both sort and hash;
create_sort_agg_path, which handles the sort case for non-partial
paths only; and create_hash_agg_path, which handles the hash case for
non-partial paths only.  This leads to the following code in 0007:

+   /* Full grouping paths */
+
+   if (try_parallel_aggregation)
+   {
+   Assert(extra->agg_partial_costs &&
extra->agg_final_costs);
+   create_partial_agg_path(root, input_rel,
grouped_rel, target,
+
 partial_target, extra->agg_partial_costs,
+
 extra->agg_final_costs, gd, can_sort,
+
 can_hash, (List *) extra->havingQual);
+   }
+
+   if (can_sort)
+   create_sort_agg_path(root, input_rel,
grouped_rel, target,
+
partial_target, agg_costs,
+
extra->agg_final_costs, gd, can_hash,
+
dNumGroups, (List *) extra->havingQual);
+
+   if (can_hash)
+   create_hash_agg_path(root, input_rel,
grouped_rel, target,
+
partial_target, agg_costs,
+
extra->agg_final_costs, gd, dNumGroups,
+(List
*) extra->havingQual);

That looks strange -- you would expect to see either "sort" and "hash"
cases here, or maybe "partial" and "non-partial", or maybe all four
combinations, but seeing three things here looks surprising.  I think
the solution is just to create a single function that does both the
work of create_sort_agg_path and the work of create_hash_agg_path
instead of having two separate functions.

A related thing that is also surprising is that 0007 manages to reuse
create_partial_agg_path for both the isPartialAgg and non-isPartialAgg
cases -- in fact, the calls appear to be identical, and could be
hoisted out of the "if" statement -- but create_sort_agg_path and
create_hash_agg_path do not get reused.  I think you should see
whether you can define the new combo function that can be used for
both cases.  The logic looks very similar, and I'm wondering why it
isn't more similar than it is; for instance, create_sort_agg_path
loops over the input rel's pathlist, but the code for
isPartialAgg/can_sort seems to consider only the cheapest path.  If
this is correct, it needs a comment explaining it, but I don't see why
it should be correct.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: proposal: alternative psql commands quit and exit

[Chapman Flack]

On 01/15/18 16:32, David Fetter wrote:
> On Mon, Jan 15, 2018 at 01:24:45PM -0500, Chapman Flack wrote:
>> On 01/15/18 11:56, David Fetter wrote:
 And while trying to find the EOF setting in libreadline might get
 messy, you're already assuming that ctrl-C hasn't been knobbled
 using stty intr. Unless you want to go searching for that too?
>>
>> So, it's a thing that can happen.
> 
> Of course it is. It's just not a thing I believe we should perturb
> our code or documentation to accommodate.

So ISTM that the best way to have code/documentation that (a) isn't
perturbed, and (b) doesn't tell people things that might be wrong,
would be (at least for this patch) to focus the help text on things
PostgreSQL controls (you're in a quoted string, here's how to end it;
you're in a multiline command, here's how to finish or reset it, etc.).

*Maybe* it could speak vaguely about your end-of-file keystroke,
or interrupt keystroke (there could well be a better way to say that),
which would be good enough now for people who know what those are,
and *maybe* that could be seen as opening the door to a future patch
if someone wanted to code up OS-specific checks and add "(which seems
to be ^D)", "(which seems to be ^Z)", etc. That would perturb the code
a bit, but the decision whether that patch would be worthwhile could
be deferred until there was someone interested in working on it.

-Chap

Re: master make check fails on Solaris 10

[Tom Lane]

Marina Polyakova  writes:
> On 13-01-2018 21:10, Tom Lane wrote:
>> I'm not sure there's much we can do about this.  Dropping the use
>> of the alignment spec isn't a workable option.  If there were a
>> simple way for configure to detect that the compiler generates bad
>> code for that, we could have it do so and reject use of __int128,
>> but it'd be up to you to come up with a workable test.

> I'll think about it..

Attached is a possible test program.  I can confirm it passes on a
machine with working __int128, but I have no idea whether it will
detect the problem on yours.  If not, maybe you can tweak it?

regards, tom lane

#include 
#include 

/* GCC, Sunpro and XLC support aligned */
#if defined(__GNUC__) || defined(__SUNPRO_C) || defined(__IBMC__)
#define pg_attribute_aligned(a) __attribute__((aligned(a)))
#endif

typedef __int128 int128a
#if defined(pg_attribute_aligned)
pg_attribute_aligned(8)
#endif
;

/*
 * These are globals to discourage the compiler from folding all the
 * arithmetic tests down to compile-time constants.  We do not have
 * convenient support for 128bit literals at this point...
 */
struct glob128
{
	__int128	start;
	char		pad;
	int128a		a;
	int128a		b;
	int128a		c;
	int128a		d;
} g = {0, 'p', 48828125, 97656255, 0, 0};

int
main()
{
	if (offsetof(struct glob128, a) < 17 ||
		offsetof(struct glob128, a) > 24)
	{
		printf("wrong alignment, %d\n", (int) offsetof(struct glob128, a));
		return 1;
	}
	g.a = (g.a << 12) + 1;		/* 2001 */
	g.b = (g.b << 12) + 5;		/* 4005 */
	/* use the most relevant arithmetic ops */
	g.c = g.a * g.b;
	g.d = (g.c + g.b) / g.b;
	/* return different values, to prevent optimizations */
	if (g.d != g.a + 1)
	{
		printf("wrong arithmetic result\n");
		return 1;
	}
	printf("A-OK!\n");
	return 0;
}

Re: jsonpath

[Andrew Dunstan]

On 01/10/2018 05:42 PM, Nikita Glukhov wrote:
> Attached new 8th version of jsonpath related patches. Complete
> documentation is still missing.
>
> The first 4 small patches are necessary datetime handling in jsonpath:
> 1. simple refactoring, extracted function that will be used later in
> jsonpath
> 2. throw an error when the input or format string contains trailing
> elements
> 3. avoid unnecessary cstring to text conversions
> 4. add function for automatic datetime type recognition by the
> presence of formatting components
>
> Should they be posted in a separate thread?
>


The first of these refactors the json/jsonb timestamp formatting into a
single function, removing a lot of code duplication. The involves
exposing time2tm() and timetz2tm(). I don't think that's a tragedy, so
unless there is any objection I propose to commit it shortly.

The next three expose a bit more of the date/time API. I'm still
reviewing those.

cheers

andrew

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

Re: [HACKERS] Replication status in logical replication

[Masahiko Sawada]

On Sun, Jan 14, 2018 at 12:43 AM, Simon Riggs  wrote:
> On 9 January 2018 at 04:36, Masahiko Sawada  wrote:
>
>>> This patch appears to cause this DEBUG1 message
>>>
>>> "standby \"%s\" has now caught up with primary"
>>>
>>> which probably isn't the right message, but might be OK to backpatch.
>>>
>>> Thoughts on better wording?
>>>
>>
>> I think that this DEBUG1 message appears when sent any WAL after
>> caught up even without this patch. This patch makes this message
>> appear at a properly timing. Or am I missing something?
>
> We're not talking about standbys, so the message is incorrect.
>

Ah, I understood. How about "\"%s\"  has now caught up with upstream server"?

Regards,

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

Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

(2018/01/16 1:47), Robert Haas wrote:

On Mon, Jan 15, 2018 at 3:09 AM, Etsuro Fujita
  wrote:

Yeah, but I don't think the above example is good enough to explain that,
because I think the bar/baz join would produce at most one tuple in an EPQ
recheck since we would have only one EPQ tuple for both bar and baz in that
recheck, and the join is inner.  I think such an example would probably be
given e.g., by a modified version of the SQL where we have a full join of
bar and baz, not an inner join.


Hmm, I was thinking that bar and baz wouldn't be constrained to return
just one tuple in that case, but I'm wrong: there would just be one
tuple per relation in that case.  However, that would also be true for
a full join, wouldn't it?


Consider:

postgres=# create table bar (a int, b text);
postgres=# create table baz (a int, b text);
postgres=# insert into bar values (1, 'bar');
postgres=# insert into baz values (2, 'baz');
postgres=# select * from bar full join baz on bar.a = baz.a;
 a |  b  | a |  b
---+-+---+-
 1 | bar |   |
   | | 2 | baz
(2 rows)

Both relations have one tuple, but the full join produces two join 
tuples.  I think it would be possible that something like this happens 
when executing a local join plan for a foreign join that performs a full 
join remotely.



Regardless of that, the patch fixes the reported problem with very
little code change, and somebody can always improve it further later.


Agreed.

Best regards,
Etsuro Fujita

Re: [HACKERS] postgres_fdw bug in 9.6

[Tom Lane]

Etsuro Fujita  writes:
> (2018/01/16 1:47), Robert Haas wrote:
>> Hmm, I was thinking that bar and baz wouldn't be constrained to return
>> just one tuple in that case, but I'm wrong: there would just be one
>> tuple per relation in that case.  However, that would also be true for
>> a full join, wouldn't it?

> Consider:

> postgres=# create table bar (a int, b text);
> postgres=# create table baz (a int, b text);
> postgres=# insert into bar values (1, 'bar');
> postgres=# insert into baz values (2, 'baz');
> postgres=# select * from bar full join baz on bar.a = baz.a;
>   a |  b  | a |  b
> ---+-+---+-
>   1 | bar |   |
> | | 2 | baz
> (2 rows)

> Both relations have one tuple, but the full join produces two join 
> tuples.  I think it would be possible that something like this happens 
> when executing a local join plan for a foreign join that performs a full 
> join remotely.

Doesn't really matter though, does it?  Each of those join rows will
be processed as a separate EPQ event.

regards, tom lane

Re: polymorphic parameters limits - correct solution?

[Alvaro Herrera]

Pavel Stehule wrote:
> Hi
> 
> I played with introduction of new pair of Polymorphic Parameters - like
> anyXelement and anyXarray. Now, I don't think so enhancing PP is good way
> now. Without significant redesign there are not practical append more code
> there.
> 
> Why this is a issue? The extension's authors are not able to specify result
> type without enumeration of all possible function signatures. Similar
> situation is in argument processing - there are workaround based on "any"
> type.

I cannot offer any advice for your "helper" (because I don't really
understand what the helper does), but I am reminded of this old thread:
https://www.postgresql.org/message-id/flat/20090908161210.GD549%40alvh.no-ip.org

... in which datatype "any" was one of the offered tools, and also from
whence the format() function sprung.  Nice ...

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: [HACKERS] Restricting maximum keep segments by repslots

[Robert Haas]

On Mon, Jan 15, 2018 at 1:05 AM, Kyotaro HORIGUCHI
 wrote:
>> >>  patch -p1 gives some "Stripping trailing CRs from patch"
>> >>  messages for me, but applied to current HEAD and builds. After
>> >
>> > Hmm. I wonder why I get that complaint so often. (It's rather
>> > common? or caused by the MIME format of my mail?) I'd say with
>> > confidence that it is because you retrieved the patch file on
>> > Windows mailer.
>> I use Debian and web based mailer. Hm, i wget patches from links here 
>> https://www.postgresql.org/message-id/flat/20180111.155910.26212237.horiguchi.kyotaro%40lab.ntt.co.jp
>>  - applies clean both last and previous messages. Its strange.
>
> Thanks for the information. The cause I suppose is that *I*
> attached the files in *text* MIME type. I taught my mailer
> application to use "Application/Octet-stream" instead and that
> should make most (or all) people here happy.

Since the "Stripping trailing CRs from patch" message is totally
harmless, I'm not sure why you should need to devote any effort to
avoiding it.  Anyone who gets it should just ignore it.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: [HACKERS] Restricting maximum keep segments by repslots

[Tom Lane]

Robert Haas  writes:
> Since the "Stripping trailing CRs from patch" message is totally
> harmless, I'm not sure why you should need to devote any effort to
> avoiding it.  Anyone who gets it should just ignore it.

Not sure, but that might be another situation in which "patch"
works and "git apply" doesn't.  (Feeling too lazy to test it...)

regards, tom lane

Re: WIP: a way forward on bootstrap data

[John Naylor]

On 1/14/18, Tom Lane  wrote:
>  So, for each catalog header pg_foo.h, there would be a
> generated file, say pg_foo_d.h, containing:
>
> * Natts_ and Anum_ macros for pg_foo
>
> * Any EXPOSE_TO_CLIENT_CODE sections copied from pg_foo.h
>
> * Any OID-value macros for entries in that catalog

I'm on board in principle, but I have some questions:

How do we have the makefiles gracefully handle 62 generated headers
which need to be visible outside the backend? Can I generalize the
approach I took for the single OIDs file I had, or is that not even
the right way to go? (In short, I used a new backend make target that
was invoked in src/common/Makefile - the details are in patch v6-0006)

If we move fmgr oid generation here as you suggested earlier, I
imagine we don't want to create a lot of #include churn. My idea is to
turn src/include/utils/fmgroids.h into a static file that just
#includes catalog/pg_proc_d.h. Thoughts?

And I'm curious, what is "_d" intended to convey?

(While I'm thinking outloud, I'm beginning to think that these headers
lie outside the scope of genbki.pl, and belong in a separate script.)

-John Naylor

Re: [HACKERS] Race between SELECT and ALTER TABLE NO INHERIT

[Kyotaro HORIGUCHI]

Hello, sorry for my late reply.

At Wed, 10 Jan 2018 14:56:49 -0500, Tom Lane  wrote in 
<26017.1515614...@sss.pgh.pa.us>
> I think that there might be a much simpler solution to this, which
> is to just remove make_inh_translation_list's tests of attinhcount,
> as per attached.  Those are really pretty redundant: since we are
> matching by column name, the unique index on pg_attribute already
> guarantees there is at most one matching column.  I have a feeling

My thought were restricted to the same behavior as the drop case,
but Tom's solution is also fine for me. I agree to the point that
the colums with the same name in a inheritance tree are safely
assumed to be in a inheritance relationship. (Assuming everything
is consistent.)

At Fri, 12 Jan 2018 15:52:08 -0500, Tom Lane  wrote in 
<15881.1515790...@sss.pgh.pa.us>
> I wrote:
> > I think that there might be a much simpler solution to this, which
> > is to just remove make_inh_translation_list's tests of attinhcount,
> > as per attached.
> 
> I went ahead and pushed that, with an isolation test based on the
> one Horiguchi-san submitted but covering a few related cases.

Thank you for commiting it.

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: [HACKERS] postgres_fdw bug in 9.6

[Etsuro Fujita]

(2018/01/16 11:17), Tom Lane wrote:

Etsuro Fujita  writes:

(2018/01/16 1:47), Robert Haas wrote:

Hmm, I was thinking that bar and baz wouldn't be constrained to return
just one tuple in that case, but I'm wrong: there would just be one
tuple per relation in that case.  However, that would also be true for
a full join, wouldn't it?



Consider:



postgres=# create table bar (a int, b text);
postgres=# create table baz (a int, b text);
postgres=# insert into bar values (1, 'bar');
postgres=# insert into baz values (2, 'baz');
postgres=# select * from bar full join baz on bar.a = baz.a;
   a |  b  | a |  b
---+-+---+-
   1 | bar |   |
 | | 2 | baz
(2 rows)



Both relations have one tuple, but the full join produces two join
tuples.  I think it would be possible that something like this happens
when executing a local join plan for a foreign join that performs a full
join remotely.


Doesn't really matter though, does it?  Each of those join rows will
be processed as a separate EPQ event.


I assume that such a local join plan is executed as part of a FOR UPDATE 
query like the one shown by Robert (the bar/baz foreign join part in 
that query), so I am thinking that those join rows will be processed as 
a single event.


Best regards,
Etsuro Fujita

Re: WIP: a way forward on bootstrap data

[Tom Lane]

John Naylor  writes:
> On 1/14/18, Tom Lane  wrote:
>> So, for each catalog header pg_foo.h, there would be a
>> generated file, say pg_foo_d.h, containing:
>> * Natts_ and Anum_ macros for pg_foo
>> * Any EXPOSE_TO_CLIENT_CODE sections copied from pg_foo.h
>> * Any OID-value macros for entries in that catalog

> I'm on board in principle, but I have some questions:

> How do we have the makefiles gracefully handle 62 generated headers
> which need to be visible outside the backend?

There are other people around here who are better make wizards than I, but
I'm sure this is soluble.  A substitution like $(CATALOG_HEADERS:_d.h=.h)
might get you started.  (It looks like CATALOG_HEADERS would have to be
separated out of POSTGRES_BKI_SRCS, but that's easy.)

> If we move fmgr oid generation here as you suggested earlier, I
> imagine we don't want to create a lot of #include churn. My idea is to
> turn src/include/utils/fmgroids.h into a static file that just
> #includes catalog/pg_proc_d.h. Thoughts?

Yeah ... or vice versa.  I don't know if touching the way fmgroids.h is
built is worthwhile.  Certainly, if we'd built all this to begin with
we'd have unified pg_proc.h's OID macro handling with the other catalogs,
but we didn't and that might not be worth changing.  I'm not strongly
convinced either way.

> And I'm curious, what is "_d" intended to convey?

I was thinking "#define" or "data".  You could make as good a case for
"_g" for "generated", or probably some other choices.  I don't have a
strong preference; but I didn't especially like your original suggestion
of "_sym", because that seemed like it would invite confusion with
possible actual names for catalogs.  A one-letter suffix seems less
likely to conflict with anything anybody would think was a good choice
of catalog name.

> (While I'm thinking outloud, I'm beginning to think that these headers
> lie outside the scope of genbki.pl, and belong in a separate script.)

Maybe, but the conditions for regenerating these files would be exactly
the same as for the .bki file, no?  So we might as well just have one
script do both, rather than writing duplicative rules in the Makefiles
and the MSVC scripts.

regards, tom lane

Re: [HACKERS] Proposal: Local indexes for partitioned table

[David Rowley]

On 12 January 2018 at 07:52, Alvaro Herrera  wrote:
> The delta patch turned out to have at least one stupid bug, and a few
> non-stupid bugs.  Here's an updated version that should behave
> correctly, and addresses all reported problems.

Thanks for updating the patch.

I've just made another pass over the patch and have a few more comments.

1. Expression varattnos don't seem to get translated correctly.
drop table p;
create table p (a int, b int) partition by list (a);
create table p1 (b int, a int);
alter table p attach partition p1 for values in(1);
create index on p (abs(a));
\d p1
 Table "public.p1"
 Column |  Type   | Collation | Nullable | Default
+-+---+--+-
 b  | integer |   |  |
 a  | integer |   |  |
Partition of: p FOR VALUES IN (1)
Indexes:
"p1_abs_idx" btree (abs(b))

CompareIndexInfo() seems to validate existing indexes correctly.

Can you also write a test for this?

2. No stats are gathered for the partitioned expression index.

drop table p;
create table p (a int, b int) partition by range (a);
create table p1 (b int, a int);
alter table p attach partition p1 for values from (1) to (1001);
create index on p1 (abs(a));
create index on p (abs(a));
insert into p select x,x from generate_series(1,1000) x;
analyze p, p1;

select * from pg_statistic where starelid = 'p_abs_idx'::regclass;
select * from pg_statistic where starelid = 'p1_abs_idx'::regclass;

3. deadlocking: I've not yet debugged this to see if this can be avoided.

drop table p;
create table p (a int not null) partition by list (a);
create table p1 partition of p for values in(1);
insert into p1 select 1 from generate_Series(1,1000);

-- Session 1:
create index concurrently on p1 (a);

-- Session 2:
create index on p (a);

-- Session 1:
ERROR:  deadlock detected
DETAIL:  Process 10860 waits for ShareLock on virtual transaction
3/97; blocked by process 7968.
Process 7968 waits for ShareLock on relation 90492 of database 12342;
blocked by process 10860.
HINT:  See server log for query details.

4. nparts initialized twice in DefineIndex()

int nparts = partdesc->nparts;
Oid*part_oids;
TupleDesc parentDesc;
bool invalidate_parent = false;

nparts = partdesc->nparts;

5. In DefineIndex, should the following have a heap_freetuple(newtup); ?

newtup = heap_copytuple(tup);
((Form_pg_index) GETSTRUCT(newtup))->indisvalid = false;
CatalogTupleUpdate(pg_index, &tup->t_self, newtup);
ReleaseSysCache(tup);
heap_close(pg_index, RowExclusiveLock);

6. Header comment in ReindexPartitionedIndex() claims it "obtains the
list of children and releases the lock on parent before applying
reindex on each child.", but it does not do that. It just returns an
error.

7. I see you changed StoreSingleInheritance to have the seqnumber as
int32 instead of int16, but StoreCatalogInheritance1 still has an
int16 seqNumber parameter. Maybe that should be fixed independently
and backpatched?

8. ATExecCmd: You've added an Assert() in the DETACH PARTITION case to
ensure we're working with a table, but what makes you certain that the
"else" case on the ATTACH PARTITION is always going to get a
partitioned index?

Maybe it would be better to just move the Assert()s into the function
being called?

9. Error details claim that p2_a_idx is not a partition of p.
Shouldn't it say table "p2" is not a partition of "p"?

DROP TABLE p;
CREATE TABLE p (a INT NOT NULL, b INT NOT NULL) PARTITION BY LIST (a);
CREATE TABLE p1 PARTITION OF p FOR VALUES IN(1) PARTITION BY LIST (b);
CREATE TABLE p2 PARTITION OF p1 FOR VALUES IN(1);

CREATE INDEX p_a_idx ON ONLY p (a);

CREATE INDEX p2_a_idx ON p2 (a);
ALTER INDEX p_a_idx ATTACH PARTITION p2_a_idx;
ERROR:  cannot attach index "p2_a_idx" as a partition of index "p_a_idx"
DETAIL:  Index "p2_a_idx" is not on a partition of table "p".

10. You've added code to get_relation_info() to handle partitioned
indexes, but that code is skipped due to:

/*
* Make list of indexes.  Ignore indexes on system catalogs if told to.
* Don't bother with indexes for an inheritance parent, either.
*/
if (inhparent ||

The new code should either be removed, or you should load the index
list for a partitioned table.

11. In ProcessUtilitySlow(), the following if needs to check if we're
dealing with a partitioned table:

/* Also, lock any descendant tables if recursive */
if (stmt->relation->inh)
inheritors = find_all_inheritors(relid, lockmode, NULL);

Otherwise we will (perhaps harmlessly) lock children in the following case:

DROP TABLE p;
CREATE TABLE p (a INT NOT NULL, b INT NOT NULL);
CREATE TABLE p1 (a INT NOT NULL, b INT NOT NULL) INHERITS (p);
CREATE INDEX ON p (a);

> I'll set this as Ready for Committer now.

Will set back to waiting on author until the above things are addressed.


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

Re: [HACKERS] UPDATE of partition key

[David Rowley]

On 16 January 2018 at 01:09, Robert Haas  wrote:
> On Sun, Jan 14, 2018 at 6:57 AM, Amit Khandekar  
> wrote:
>> Even where partitions are present, in the usual case where there are
>> Instead of a bool array, we can even make it a Bitmapset. But I think
>> access would become slower as compared to array, particularly because
>> it is going to be a heavily used function.
>
> It probably makes little difference -- the Bitmapset will be more
> compact (which saves time) but involve function calls (which cost
> time).

I'm not arguing in either direction, but you'd also want to factor in
how Bitmapsets only allocate words for the maximum stored member,
which might mean multiple realloc() calls resulting in palloc/memcpy
calls. The array would just be allocated in a single chunk, although
it would be more memory and would require a memset too, however,
that's likely much cheaper than the palloc() anyway.

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

Re: polymorphic parameters limits - correct solution?

[Pavel Stehule]

2018-01-16 3:21 GMT+01:00 Alvaro Herrera :

> Pavel Stehule wrote:
> > Hi
> >
> > I played with introduction of new pair of Polymorphic Parameters - like
> > anyXelement and anyXarray. Now, I don't think so enhancing PP is good way
> > now. Without significant redesign there are not practical append more
> code
> > there.
> >
> > Why this is a issue? The extension's authors are not able to specify
> result
> > type without enumeration of all possible function signatures. Similar
> > situation is in argument processing - there are workaround based on "any"
> > type.
>
> I cannot offer any advice for your "helper" (because I don't really
> understand what the helper does), but I am reminded of this old thread:
> https://www.postgresql.org/message-id/flat/20090908161210.GD549%40alvh.
> no-ip.org


yes - it is same issue. There are possible two ways

1. with polymorphic types
2. with some aux functions that dynamically generate current function's
signature - used in core (coalesce, greather, ...)

@1 is difficult when you can implement different strategies about work with
arguments

a) depends on first
b) depends on common type
c) depends on second

So I am thinking so the best possibility is allow same functionality that
we use in core to extension authors.

Regards

Pavel



>
> ... in which datatype "any" was one of the offered tools, and also from
> whence the format() function sprung.  Nice ...
>
> --
> Álvaro Herrerahttps://www.2ndQuadrant.com/
> PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>

Re: ALTER TABLE ADD COLUMN fast default

[David Rowley]

On 2 January 2018 at 05:01, Andrew Dunstan
 wrote:
> New version of the patch that fills in the remaining piece in
> equalTupleDescs.

This no longer applies to current master. Can send an updated patch?

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

Re: New gist vacuum.

[Andrey Borodin]

Hi, Alexander!

Many thanks for looking into patches!
A little bit later I will provide answer in other branch of discussion.

> 15 янв. 2018 г., в 23:34, Alexander Korotkov  
> написал(а):
> 
> I do not know, should I register separate commitfest entry? The code is very 
> close to main GiST VACUUM patch, but solves a bit different problem.
> 
> Yes, I think it deserves separate commitfest entry.  Despite it's related to 
> GiST VACUUM, it's a separate fix.
Ok, done. https://commitfest.postgresql.org/17/1483
> I've made small improvements to this patch: variable naming, formatting, 
> comments.
Great, thanks!
> BTW, do we really need to set shouldCount depending on whether we receive 
> stats argument or not?  What if we always set shouldCount as in the first 
> branch of "if"?
>  
>   shouldCount = !heap_attisnull(rel->rd_indextuple, 
> Anum_pg_index_indpred) ||
>  info->estimated_count;
We do not need to count if we have exact count from heap and this index is not 
partial. ITSM that it is quite common case.

Best regards, Andrey Borodin.

Re: [HACKERS] taking stdbool.h into use

[Michael Paquier]

On Thu, Jan 11, 2018 at 06:40:05PM -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
>> That leaves the uses in rowtypes.c.  Those were introduced as a
>> portability fix by commit 4cbb646334b.  I'm curious why these are
>> necessary.  The Datums they operate come from heap_deform_tuple(), which
>> gets them from fetchatt(), which does run all pass-by-value values
>> through the very same GET_X_BYTES() macros (until now).  I don't see
>> where those dirty upper bits would be coming from.
> 
> I don't see it either.  I think the actually useful parts of that patch
> were to declare record_image_cmp's result correctly as int rather than
> bool, and to cope with varlena fields of unequal size.  Unfortunately
> there seems to be no contemporaneous mailing list discussion, so
> it's not clear what Kevin based this change on.

This was a hotfix after a buildfarm breakage, the base commit being
f566515.

> Want to try reverting the GET_X_BYTES() parts of it and see if the
> buildfarm complains?

So, Peter, are you planning to do so?

> Note if that if we simplify the GetDatum macros, it's possible that
> record_image_cmp would change behavior, since it might now see signed not
> unsigned values depending on whether the casts do sign extension or not.
> Not sure if that'd be a problem.

Based on the patch series in
https://www.postgresql.org/message-id/d86ec1f4-941c-e702-b05a-748ea2e59...@2ndquadrant.com,
the next thing that could be shipped is 0003 in my opinion, as 0002 has
already been pushed.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] Replication status in logical replication

[Michael Paquier]

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.
--
Michael


signature.asc
Description: PGP signature

Re: [HACKERS] generated columns

[Michael Paquier]

On Wed, Dec 27, 2017 at 12:31:05PM -0500, Peter Eisentraut wrote:
> On 9/12/17 15:35, Jaime Casanova wrote:
>> On 10 September 2017 at 00:08, Jaime Casanova
>>  wrote:
>>>
>>> During my own tests, though, i found some problems:
> 
> Here is an updated patch that should address the problems you have
> found.

Could you rebase the patch? I am planning to review it but there are
conflicts in genbki.pl.
--
Michael


signature.asc
Description: PGP signature

Setting BLCKSZ 4kB

[sanyam jain]

Hi,

I am trying to solve WAL flooding due to FPWs.


What are the cons of setting BLCKSZ as 4kB?


When saw the results published on 
http://blog.coelho.net/database/2014/08/17/postgresql-page-size-for-SSD-2.html

4kB page is giving better performance in comparison to 8kB except when tested 
with 15kB row size.


Does turning off FPWs will be safe if BLCKSZ is set to 4kB given page size of 
file system is 4kB?


Thanks,

Sanyam Jain

Re: Enhance pg_stat_wal_receiver view to display connected host

[Haribabu Kommi]

[ Including Hackers as earlier mail mistakenly removed it ]

On Tue, Jan 16, 2018 at 2:55 PM, Michael Paquier 
wrote:

> On Mon, Jan 15, 2018 at 05:51:58PM +1100, Haribabu Kommi wrote:
> > Instead of effective_conninfo, I changed the column name as
> > remote_serve_info and
> > display only the host, hostaddr and port details. These are the only
> values
> > that differs
> > with each remote connection.
>
> I agree that it is pointless to show duplication between both
> strings. The differences would be made harder to catch.
>
> +PQconninfoOption *
> +PQeffectiveConninfo(PGconn *conn)
> +{
> +   PQExpBufferData errorBuf;
> +   PQconninfoOption *connOptions;
> +
> +   if (conn == NULL)
> +   return NULL;
>
> Shouldn't this check for CONNECTION_BAD as well and return NULL?
>

OK. I will update it.


> If I use something like "port=5432 host=/tmp" as connection string, then
> PQeffectiveConninfo gives me the following with hostaddr being weird:
> host=/tmp hostaddr=(null) port=5432
> This should show an empty hostaddr value.
>

I will correct it.


> + remote_server_info
> + text
> + 
> +  Effective remote server connection info string used by this WAL
> receiver.
> + 
> "Effective" is not a precise term. What about just telling that this is
> the set of parameters used for hte active connection, and that this
> value should be the one used when using multiple host, hostaddr, and
> ports.
>

OK. I will update it.

Note that I still find this API confusing, it seems to me that just
> sorting out the confusion problems with PQhost and then use it would be
> more simple.
>

OK, Understood. Even if the confusion problems with PQhost that are
discussed in [1] are solved, we need two new API's that are required to\
display the proper remote server details.

PQhostNoDefault - Similar like PQhost but doesn't return default host
details.

Displaying default value always some confuse even if the user doesn't
provide
the host details, so to avoid that confusion, we need this function.

PQhostaddr - Return hostaddr used in the connection.

Without PQhostaddr() function, for the connections where the host is not
specified, it will be difficult to find out to remote server.

With the above two new API's we can display either string or individual
columns
representation of remote server.

comments?

Regards,
Hari Babu
Fujitsu Australia

Re: [HACKERS] Deadlock in XLogInsert at AIX

[Michael Paquier]

On Fri, Feb 03, 2017 at 12:26:50AM +, Noah Misch wrote:
> On Wed, Feb 01, 2017 at 02:39:25PM +0200, Heikki Linnakangas wrote:
>> @@ -73,11 +73,19 @@ pg_atomic_compare_exchange_u32_impl(volatile 
>> pg_atomic_uint32 *ptr,
>>  static inline uint32
>>  pg_atomic_fetch_add_u32_impl(volatile pg_atomic_uint32 *ptr, int32 add_)
>>  {
>> +uint32  ret;
>> +
>>  /*
>> - * __fetch_and_add() emits a leading "sync" and trailing "isync", 
>> thereby
>> - * providing sequential consistency.  This is undocumented.
>> + * Use __sync() before and __isync() after, like in compare-exchange
>> + * above.
>>   */
>> -return __fetch_and_add((volatile int *)&ptr->value, add_);
>> +__sync();
>> +
>> +ret = __fetch_and_add((volatile int *)&ptr->value, add_);
>> +
>> +__isync();
>> +
>> +return ret;
>>  }
> 
> Since this emits double syncs with older xlc, I recommend instead replacing
> the whole thing with inline asm.  As I opined in the last message of the
> thread you linked above, the intrinsics provide little value as abstractions
> if one checks the generated code to deduce how to use them.  Now that the
> generated code is xlc-version-dependent, the port is better off with
> compiler-independent asm like we have for ppc in s_lock.h.

Could it be cleaner to just use __xlc_ver__ to avoid double syncs on
past versions? I think that it would make the code more understandable
than just listing directly the instructions. As there have been other
bug reports from Tony Reix who has been working on AIX with XLC 13.1 and
that this thread got lost in the wild, I have added an entry in the next
CF:
https://commitfest.postgresql.org/17/1484/

As Heikki is not around these days, Noah, could you provide a new
version of the patch? This bug has been around for some time now, it
would be nice to move on.. I think I could have written patches myself,
but I don't have an AIX machine at hand. Of course not with XLC 13.1.
--
Michael


signature.asc
Description: PGP signature

Re: Rangejoin rebased

[Jeff Davis]

On Fri, Jan 12, 2018 at 11:02 AM, Alexander Kuzmenkov
 wrote:
> The sort order isn't right for the join, it seems. I remember having similar
> troubles with my full merge join implementation. I tried filtering
> unsuitable paths in try_mergejoin_path, but that was not quite enough. The
> planner tries to use only one sort direction to limit the number of path, so
> the path we need might not be there at all. This optimization was added in
> commit 834ddc62, see right_merge_direction(). Sadly, I have no idea how to
> solve this.

Interesting problem, thank you. My first reaction was to hack
right_merge_direction() to recognize the range opfamily in the ASC
direction as always potentially useful. But what's happening is that,
when the inputs are already forced to be sorted DESC, as in your
example, there is still no appropriate pathkey. So the problem is
reproducible with no ORDER BY clause at all.

My proposed fix is to make an internal opfamily identical to the
external one, such that it's not recognized as part of the same EC,
and the planner won't try to eliminate it. It loses out on potential
optimizations, but those are mostly theoretical since the btree
opclass ordering for ranges is not very interesting to a user.

I notice that window functions seem to handle these cases better,
maybe that approach would work for your full join patch? I haven't
investigated that yet.

 Regards,
   Jeff Davis

Re: PostgreSQL crashes with SIGSEGV

[Aleksandr Parfenov]

The following review has been posted through the commitfest application:
make installcheck-world:  tested, passed
Implements feature:   tested, passed
Spec compliant:   tested, passed
Documentation:tested, passed

Hi,

All information is related to WIP-tuplesort-memcontext-fix.patch.
The patch applies and fixes the bug on REL9_6_STABLE and LGTM.

However, REL9_5_STABLE affected as well and the patch doesn't applicable to it.
But it may be another entry because the difference in tuplesort may cause some 
changes in the fix too.

The new status of this patch is: Ready for Committer