Re: Debian mips: Failed test 'Check expected t_009_tbl data on standby'

2018-10-12 Thread Christoph Berg
Re: Michael Paquier 2018-10-12 <20181012002520.gb26...@paquier.xyz>
> Do you still have the logs of the previous run for the standby?

Sorry, all I have is the (link to) the build log in the original
posting. I can run some tests on the mips porter box if you have any
ideas for things to try.

What's missing is a way to determine which .log files to show in case
of a test failure. At the moment I'm showing the latest 3 by mtime:

unset MAKELEVEL; if ! make -C build check-world 
EXTRA_REGRESS_OPTS='--port=$(shell perl -le 'print 1024 + int(rand(64000))')'; 
then \
for l in `find build \( -name regression.diffs -o -name initdb.log 
-o -name postmaster.log \) | perl -we 'print map { "$$_\n"; } sort { (stat 
$$a)[9] <=> (stat $$b)[9] } map { chomp; $$_; } <>' | tail -3`; do \
echo " $$l "; \
cat $$l; \
done; \
case $(DEB_HOST_ARCH) in \
hurd-*|kfreebsd-*) exit 0 ;; \
*) exit 1 ;; \
esac; \
fi

Maybe I should change that to 10. Or just show all given it happens
only for failing builds. (In case anyone is wondering: hurd doesn't
have semaphores, and kfreebsd always fails the plperl tests.)

Christoph



[PATCH] Change simple_heap_insert() to a macro

2018-10-12 Thread Andrey Klychkov

Hi, Hackers
Studying another question I noticed a small point for optimization.
In the src/backend/access/heap/heapam.c we have the function:
- * simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
- return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}
I changed it to a macro. See the attached patch.
I will be grateful if someone look at this.
Thank you! -- 
Regards,
Andrey Klychkovdiff --git a/src/backend/access/heap/heapam.c b/src/backend/access/heap/heapam.c
index fb63471..ce8cdb9 100644
--- a/src/backend/access/heap/heapam.c
+++ b/src/backend/access/heap/heapam.c
@@ -2410,6 +2410,11 @@ ReleaseBulkInsertStatePin(BulkInsertState bistate)
 /*
  *	heap_insert		- insert tuple into a heap
  *
+ *  NB: simple_heap_insert macro should be used rather than using heap_insert
+ *  directly in most places where we are modifying system catalogs.
+ *  Currently, this routine differs from heap_insert only in supplying
+ *  a default command ID and not allowing access to the speedup options.
+ *
  * The new tuple is stamped with current transaction ID and the specified
  * command ID.
  *
@@ -2987,21 +2992,6 @@ heap_multi_insert(Relation relation, HeapTuple *tuples, int ntuples,
 }
 
 /*
- *	simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most places
- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
-	return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}
-
-/*
  * Given infomask/infomask2, compute the bits that must be saved in the
  * "infobits" field of xl_heap_delete, xl_heap_update, xl_heap_lock,
  * xl_heap_lock_updated WAL records.
diff --git a/src/include/access/heapam.h b/src/include/access/heapam.h
index 40e153f..0941778 100644
--- a/src/include/access/heapam.h
+++ b/src/include/access/heapam.h
@@ -16,6 +16,7 @@
 
 #include "access/sdir.h"
 #include "access/skey.h"
+#include "access/xact.h"
 #include "nodes/lockoptions.h"
 #include "nodes/primnodes.h"
 #include "storage/bufpage.h"
@@ -107,6 +108,18 @@ typedef struct ParallelHeapScanDescData *ParallelHeapScanDesc;
  */
 #define HeapScanIsValid(scan) PointerIsValid(scan)
 
+/*
+ *	simple_heap_insert - insert a tuple
+ *
+ * Currently, this routine differs from heap_insert only in supplying
+ * a default command ID and not allowing access to the speedup options.
+ *
+ * This should be used rather than using heap_insert directly in most places
+ * where we are modifying system catalogs.
+ */
+#define simple_heap_insert(relation, tup) \
+	heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL)
+
 extern HeapScanDesc heap_beginscan(Relation relation, Snapshot snapshot,
 			   int nkeys, ScanKey key);
 extern HeapScanDesc heap_beginscan_catalog(Relation relation, int nkeys,
@@ -176,7 +189,6 @@ extern bool heap_tuple_needs_freeze(HeapTupleHeader tuple, TransactionId cutoff_
 		MultiXactId cutoff_multi, Buffer buf);
 extern bool heap_tuple_needs_eventual_freeze(HeapTupleHeader tuple);
 
-extern Oid	simple_heap_insert(Relation relation, HeapTuple tup);
 extern void simple_heap_delete(Relation relation, ItemPointer tid);
 extern void simple_heap_update(Relation relation, ItemPointer otid,
    HeapTuple tup);


Re: [PATCH] Change simple_heap_insert() to a macro

2018-10-12 Thread Heikki Linnakangas

On 12/10/2018 11:54, Andrey Klychkov wrote:

Studying another question I noticed a small point for optimization.

In the src/backend/access/heap/heapam.c we have the function:

- * simple_heap_insert - insert a tuple
- *
- * Currently, this routine differs from heap_insert only in supplying
- * a default command ID and not allowing access to the speedup options.
- *
- * This should be used rather than using heap_insert directly in most 
places

- * where we are modifying system catalogs.
- */
-Oid
-simple_heap_insert(Relation relation, HeapTuple tup)
-{
- return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
-}

I changed it to a macro. See the attached patch.


simple_heap_insert() is used in catalog updates and such. Does that have 
any measurable performance impact?


- Heikki



Re[2]: [PATCH] Change simple_heap_insert() to a macro

2018-10-12 Thread Andrey Klychkov
> simple_heap_insert() is used in catalog updates and such. Does that have
> any measurable performance impact?

I guess this change doesn't give us an incredible performance increase except 
there will no redundant function call.
I don't see any reasons against to use the proposed macro instead of this 
function.

>Пятница, 12 октября 2018, 12:16 +03:00 от Heikki Linnakangas :
>
>On 12/10/2018 11:54, Andrey Klychkov wrote:
>> Studying another question I noticed a small point for optimization.
>> 
>> In the src/backend/access/heap/heapam.c we have the function:
>> 
>> - * simple_heap_insert - insert a tuple
>> - *
>> - * Currently, this routine differs from heap_insert only in supplying
>> - * a default command ID and not allowing access to the speedup options.
>> - *
>> - * This should be used rather than using heap_insert directly in most 
>> places
>> - * where we are modifying system catalogs.
>> - */
>> -Oid
>> -simple_heap_insert(Relation relation, HeapTuple tup)
>> -{
>> - return heap_insert(relation, tup, GetCurrentCommandId(true), 0, NULL);
>> -}
>> 
>> I changed it to a macro. See the attached patch.
>
>simple_heap_insert() is used in catalog updates and such. Does that have 
>any measurable performance impact?
>
>- Heikki


-- 
Regards,
Andrey Klychkov


Re: pgsql: Add TAP tests for pg_verify_checksums

2018-10-12 Thread Michael Banck
Hi,

On Fri, Oct 12, 2018 at 03:05:43PM +0900, Michael Paquier wrote:
> On Fri, Oct 12, 2018 at 12:11:58PM +0900, Michael Paquier wrote:
> > Agreed.  I am just working on a patch for v11- which uses a
> > whitelist-based method instead of what is present now.  Reverting the
> > tests to put the buildfarm to green could be done, but that's not the
> > root of the problem.

I think that's the right solution; the online verification patch adds
even more logic to the blacklist so getting rid of it in favor of a
whitelist is +1 with me.
 
> So, I have coded this thing, and finish with the attached.  The
> following file patterns are accepted for the checksums:
> .
> _
> _.
> I have added some tests on the way to make sure that all the patterns
> get covered.  Please note that this skips the temporary files.

Maybe also add some correct (to be scanned) but non-empty garbage files
later on that it should barf on?


Michael

-- 
Michael Banck
Projektleiter / Senior Berater
Tel.: +49 2166 9901-171
Fax:  +49 2166 9901-100
Email: michael.ba...@credativ.de

credativ GmbH, HRB Mönchengladbach 12080
USt-ID-Nummer: DE204566209
Trompeterallee 108, 41189 Mönchengladbach
Geschäftsführung: Dr. Michael Meskes, Jörg Folz, Sascha Heuer

Unser Umgang mit personenbezogenen Daten unterliegt
folgenden Bestimmungen: https://www.credativ.de/datenschutz



Re: Calculate total_table_pages after set_base_rel_sizes()

2018-10-12 Thread Amit Langote
On 2018/10/11 13:45, Amit Langote wrote:
> On 2018/10/07 17:43, David Rowley wrote:
>> Amit Langote has since posted a patch to delay the RangeTblEntry
>> creation until after pruning. His patch happens to also move the
>> total_table_pages calculation, but I believe this change should be
>> made as an independent commit to anything else.  I've kept it in the
>> commitfest for that reason.
> 
> Yeah, if this patch is a win independent of the other project of delaying
> partition RTE creation, which seems to be true, I think we should go ahead
> with applying this patch.

Fwiw, I've incorporated David's patch in my own series, so that one of my
patches no longer has the code movement hunks that are in his.  I will
post the new version of my patch series like that.

Thanks,
Amit




Re: [HACKERS] Secondary index access optimizations

2018-10-12 Thread Konstantin Knizhnik



On 08.10.2018 00:16, David Rowley wrote:

On 5 October 2018 at 04:45, Konstantin Knizhnik
 wrote:

Will the following test be enough:

-- check that columns for parent table are correctly mapped to child
partition of their order doesn't match
create table paren (a int, b text) partition by range(a);
create table child_1 partition of paren for values from (0) to (10);
create table child_2 (b text, a int);
alter table paren attach partition child_2 for values from (10) to (20);
insert into paren values (generate_series(0,19), generate_series(100,119));

explain (costs off) select * from paren where a between 0 and 9;
explain (costs off) select * from paren where a between 10 and 20;
explain (costs off) select * from paren where a >= 5;
explain (costs off) select * from paren where a <= 15;

select count(*) from paren where a >= 5;
select count(*) from paren where a < 15;

drop table paren cascade;

I started looking at this to see if this test would cause a crash with
the original code, but it does not. The closest I can get is:

drop table parent;
create table parent (a bytea, b int) partition by range(a);
create table child_1 (b int, a bytea);
alter table parent attach partition child_1 for values from ('a') to ('z');
explain (costs off) select * from parent where b = 1;

But despite the varattnos of the bytea and int accidentally matching,
there's no crash due to the way operator_predicate_proof() requires
more than just the varno and varattno to match. It requires the Vars
to be equal(), which includes vartype, and those are not the same. So
the proof just fails.

In short, probably this test is doing nothing in addition to what
various other tests are doing. So given the test is unable to crash
the unfixed code, then I think it's likely not a worthwhile test to
add.

I wrote:

create table listp (a int, b int) partition by list(a);
create table listp1 partition of listp for values in(1);
create index listp_a_b_idx on listp (a,b);

and a query:

select * from listp where a = 1 order by b;

if we remove the "a = 1" qual, then listp_a_b_idx can't be used.

I had a look at what allows this query still to use the index and it's
down to pathkey_is_redundant() returning true because there's still an
equivalence class containing {a,1}. I don't quite see any reason why
it would not be okay to rely on that working, but that only works for
pathkeys. If you have a case like:

set max_parallel_workers_per_gather=0;
create table listp (a int, b int) partition by list(a);
create table listp1 partition of listp for values in(1);
insert into listp select 1,x from generate_Series(1,100) x;
create index listp_a_b_idx on listp (a,b);
vacuum analyze listp;
explain analyze select * from listp where a = 1 and b = 1;

the "a = 1" will be dropped and the index on (a,b) does not get used.

Patched results in:

postgres=# explain analyze select * from listp where a = 1 and b = 1;
  QUERY PLAN

  Append  (cost=0.00..16925.01 rows=1 width=8) (actual
time=0.019..169.231 rows=1 loops=1)
->  Seq Scan on listp1  (cost=0.00..16925.00 rows=1 width=8)
(actual time=0.017..169.228 rows=1 loops=1)
  Filter: (b = 1)
  Rows Removed by Filter: 99
  Planning Time: 0.351 ms
  Execution Time: 169.257 ms
(6 rows)

Whereas unpatched gets:

postgres=# explain analyze select * from listp where a = 1 and b = 1;
 QUERY PLAN
--
  Append  (cost=0.42..4.45 rows=1 width=8) (actual time=0.657..0.660
rows=1 loops=1)
->  Index Only Scan using listp1_a_b_idx on listp1
(cost=0.42..4.44 rows=1 width=8) (actual time=0.653..0.655 rows=1
loops=1)
  Index Cond: ((a = 1) AND (b = 1))
  Heap Fetches: 0
  Planning Time: 32.303 ms
  Execution Time: 0.826 ms
(6 rows)

so I was probably wrong about suggesting set_append_rel_size() as a
good place to remove these quals. It should perhaps be done later, or
maybe we can add some sort of marker to the qual to say it does not
need to be enforced during execution. Probably the former would be
best as we don't want to show these in EXPLAIN.

Well, I made a different conclusion from this problem (inability use of 
compound index because of redundant qual elimination).
Is it really good idea to define compound index with first key equal to 
partitioning key?
Restriction on this key in any case will lead to partition pruning. We 
do no need index for it...

In your case if we create index listp_b_idx:

 create index listp_b_idx on listp (b);

then right plan we be generated:

 Append  (cost=0.42..8.45 rows=1 width=8) (actual time=0.046..0.047 
rows=1 loops=1)
   ->  Index Scan using listp1_b_idx on listp1  (cost=0.42..8.44 rows=1 
width=8) (actu

Re: WIP: Avoid creation of the free space map for small tables

2018-10-12 Thread Amit Kapila
On Sat, Oct 6, 2018 at 12:17 AM John Naylor  wrote:
>
> -There'll need to be some performance testing to make sure there's no
> regression, and to choose a good value for the threshold. I'll look
> into that, but if anyone has any ideas for tests, that'll help this
> effort along.
>

Can you try with a Copy command which copies just enough tuples to
fill the pages equivalent to HEAP_FSM_EXTENSION_THRESHOLD?  It seems
to me in such a case patch will try each of the blocks multiple times.
  It looks quite lame that we have to try again and again the blocks
which we have just filled by ourselves but may be that doesn't matter
much as the threshold value is small.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> On 2018-10-11 17:11:47 -0400, Tom Lane wrote:
>> A compromise that occurred to me after a bit of reflection is to place
>> the necessary table-drop commands in a new regression test script that's
>> meant to be executed last, but isn't actually run by default.  Then
>> teach the cross-version-update test script to include that script via
>> EXTRA_TESTS.  Manual testing could do likewise.  Then we have a small
>> amount of pain for testing upgrades, but we lose no test coverage in
>> back branches.

> To me that seems to be more work / infrastructure than
> warranted. abstime/reltime/tinterval don't present pg_dump with any
> special challenges compared to a lot of other types we do test, no?

Well, in any case I'd say we should put the dropping commands into
a separate late-stage test script.  Whether that's run by default is a
secondary issue: if it is, somebody who wanted to test this stuff could
remove the script from their test schedule file.

regards, tom lane



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Andrew Dunstan




On 10/12/2018 10:03 AM, Tom Lane wrote:

Andres Freund  writes:

On 2018-10-11 17:11:47 -0400, Tom Lane wrote:

A compromise that occurred to me after a bit of reflection is to place
the necessary table-drop commands in a new regression test script that's
meant to be executed last, but isn't actually run by default.  Then
teach the cross-version-update test script to include that script via
EXTRA_TESTS.  Manual testing could do likewise.  Then we have a small
amount of pain for testing upgrades, but we lose no test coverage in
back branches.

To me that seems to be more work / infrastructure than
warranted. abstime/reltime/tinterval don't present pg_dump with any
special challenges compared to a lot of other types we do test, no?

Well, in any case I'd say we should put the dropping commands into
a separate late-stage test script.  Whether that's run by default is a
secondary issue: if it is, somebody who wanted to test this stuff could
remove the script from their test schedule file.





Anything that runs at the time we do the regression tests has problems, 
from my POV. If we run the drop commands then upgrading these types to a 
target <= 11 isn't tested. If we don't run them then upgrade to a 
version > 11 will fail. This is why I suggested doing it later in the 
buildfarm module that actaally does cross version upgrade testing. It 
can drop or not drop prior to running the upgrade test, depending on the 
target version.


cheers

andrew

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




Re: BUG #15425: DETACH/ATTACH PARTITION bug

2018-10-12 Thread Alvaro Herrera
Pushed, after some further refinement of the test case so that it'd
verify a few more corner case situations.

Thanks Michael.

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



Multi-insert into a partitioned table with before insert row trigger causes server crash on latest HEAD

2018-10-12 Thread Ashutosh Sharma
Hi All,

The copy command on partitioned table causes server crash when before
insert row trigger is created on one of its partition. Please find the
following test-case  to reproduce the crash.

-- create a partitioned table
create table part_copy_test  (a int, b int, c text) partition by list (b);
create table part_copy_test_a1 partition of part_copy_test for values in(1);
create table part_copy_test_a2 partition of part_copy_test for values in(2);

-- insert enough rows to allow multi-insert into a partitioned table.
copy (select x,1,'One' from generate_series(1,1000) x
union all
select x,2,'two' from generate_series(1001,1010) x
union all
select x,1,'One' from generate_series(1011,1020) x) to
'/tmp/multi_insert_data.csv';

-- create before insert row trigger on part_copy_test_a2
create function part_ins_func() returns trigger language plpgsql as
$$
begin
  return new;
end;
$$;

create trigger part_ins_trig before insert on part_copy_test_a2
for each row execute procedure part_ins_func();

-- run copy command on partitioned table.
copy part_copy_test from '/tmp/multi_insert_data.csv';

postgres=# copy part_copy_test from '/tmp/multi_insert_data.csv';
server closed the connection unexpectedly
This probably means the server terminated abnormally
before or while processing the request.
The connection to the server was lost. Attempting reset: Failed.
!>

I've spent some time looking into this issue and found that,
CopyFrom() is trying perform multi-insert for the partition that has
before insert row trigger created on it which is not expected. When a
normal table with before insert row trigger is created, CopyFrom
doesn't allow multi insert on such tables and i guess same should be
the case with table partition as well. Please correct me if i my
understanding is wrong ?

Thanks,

-- 
With Regards,
Ashutosh Sharma
EnterpriseDB:http://www.enterprisedb.com



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Tom Lane
Andrew Dunstan  writes:
> On 10/12/2018 10:03 AM, Tom Lane wrote:
>> Well, in any case I'd say we should put the dropping commands into
>> a separate late-stage test script.  Whether that's run by default is a
>> secondary issue: if it is, somebody who wanted to test this stuff could
>> remove the script from their test schedule file.

> Anything that runs at the time we do the regression tests has problems, 
> from my POV. If we run the drop commands then upgrading these types to a 
> target <= 11 isn't tested. If we don't run them then upgrade to a 
> version > 11 will fail. This is why I suggested doing it later in the 
> buildfarm module that actaally does cross version upgrade testing. It 
> can drop or not drop prior to running the upgrade test, depending on the 
> target version.

I'd like a solution that isn't impossibly difficult for manual testing
of cross-version cases, too.  That's why I'd like there to be a regression
test script that does the necessary drops.  Exactly when and how that
gets invoked is certainly open for discussion.  In the manual case
I'd imagine calling it with EXTRA_TESTS while running the setup of
the source database, if it's not run by default.  Maybe the buildfarm
module could just invoke the same script directly at a suitable point?

regards, tom lane



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Andres Freund
Hi,

On 2018-10-12 11:23:30 -0400, Andrew Dunstan wrote:
> Anything that runs at the time we do the regression tests has problems, from
> my POV. If we run the drop commands then upgrading these types to a target
> <= 11 isn't tested.

I'm asking again, what exactly do we test by having these types in the
dump? They're bog standard types, there's nothing new for pg_dump to
test with them. No weird typmod rules, no weird parse-time type mapping,
nothing?

Greetings,

Andres Freund



Re: Continue work on changes to recovery.conf API

2018-10-12 Thread Sergei Kornilov
Hello

I complete new version of this patch. I've attached it.

In this version i remove proposed recovery_target_type/recovery_target_value 
and implement set of current settings: recovery_target (immediate), 
recovery_target_name, recovery_target_time, recovery_target_xid, 
recovery_target_lsn

>>>   - trigger_file was renamed to promote_signal_file for clarify (my change, 
>>> in prev thread was removed)
>>
>>  OK to add the "promote" prefix, but I'd prefer to keep the "trigger"
>>  name. There is a lot of discussion and knowledge around that. Seems
>>  unnecessary to change.
>
> Ok, will change to promote_trigger_file
Renamed to promote_trigger_file

Possible we need something like separate xlog_guc.c and header for related 
functions and definition?

regards, Sergeidiff --git a/contrib/pg_standby/pg_standby.c b/contrib/pg_standby/pg_standby.c
index ee1fbd7..946239c 100644
--- a/contrib/pg_standby/pg_standby.c
+++ b/contrib/pg_standby/pg_standby.c
@@ -611,7 +611,7 @@ usage(void)
 	printf("  -w MAXWAITTIME max seconds to wait for a file (0=no limit) (default=0)\n");
 	printf("  -?, --help show this help, then exit\n");
 	printf("\n"
-		   "Main intended use as restore_command in recovery.conf:\n"
+		   "Main intended use as restore_command in postgresql.conf:\n"
 		   "  restore_command = 'pg_standby [OPTION]... ARCHIVELOCATION %%f %%p %%r'\n"
 		   "e.g.\n"
 		   "  restore_command = 'pg_standby /mnt/server/archiverdir %%f %%p %%r'\n");
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 3fa5efd..780f40d 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -1220,7 +1220,7 @@ SELECT pg_stop_backup();


 
- Create a recovery command file recovery.conf in the cluster
+ Create a file recovery.signal in the cluster
  data directory (see ). You might
  also want to temporarily modify pg_hba.conf to prevent
  ordinary users from connecting until you are sure the recovery was successful.
@@ -1232,10 +1232,9 @@ SELECT pg_stop_backup();
  proceed to read through the archived WAL files it needs.  Should the
  recovery be terminated because of an external error, the server can
  simply be restarted and it will continue recovery.  Upon completion
- of the recovery process, the server will rename
- recovery.conf to recovery.done (to prevent
- accidentally re-entering recovery mode later) and then
- commence normal database operations.
+ of the recovery process, the server will remove
+ recovery.signal (to prevent accidentally re-entering
+ recovery mode later) and then commence normal database operations.
 


@@ -1249,12 +1248,8 @@ SELECT pg_stop_backup();

 

-The key part of all this is to set up a recovery configuration file that
-describes how you want to recover and how far the recovery should
-run.  You can use recovery.conf.sample (normally
-located in the installation's share/ directory) as a
-prototype.  The one thing that you absolutely must specify in
-recovery.conf is the restore_command,
+The key part of all this is to set up a recovery configuration.
+The one thing that you absolutely must specify is the restore_command,
 which tells PostgreSQL how to retrieve archived
 WAL file segments.  Like the archive_command, this is
 a shell command string.  It can contain %f, which is
@@ -1316,7 +1311,7 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'

 If you want to recover to some previous point in time (say, right before
 the junior DBA dropped your main transaction table), just specify the
-required stopping point in recovery.conf.  You can specify
+required stopping point.  You can specify
 the stop point, known as the recovery target, either by
 date/time, named restore point or by completion of a specific transaction
 ID.  As of this writing only the date/time and named restore point options
@@ -1414,8 +1409,8 @@ restore_command = 'cp /mnt/server/archivedir/%f %p'
 that was current when the base backup was taken.  If you wish to recover
 into some child timeline (that is, you want to return to some state that
 was itself generated after a recovery attempt), you need to specify the
-target timeline ID in recovery.conf.  You cannot recover into
-timelines that branched off earlier than the base backup.
+target timeline ID in . You
+cannot recover into timelines that branched off earlier than the base backup.

   
 
diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 7554cba..4c79113 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3203,11 +3203,11 @@ include_dir 'conf.d'
 application_name setting of the standby, as set in the
 standby's connection information.  In case of a physical replication
 standby, this should be set in the primary_conninfo
-setting in recovery.conf; the def

Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> I'm asking again, what exactly do we test by having these types in the
> dump? They're bog standard types, there's nothing new for pg_dump to
> test with them. No weird typmod rules, no weird parse-time type mapping,
> nothing?

That's a pretty fair point.  The types' I/O functions will be exercised
well enough by the regression tests themselves, and it's hard to see what
more test coverage is gained by including them in pg_dump/pg_upgrade
testing.  Maybe we should just drop those tables and be done with it.

regards, tom lane



constraints names on partitions

2018-10-12 Thread Alvaro Herrera
Hello

I just realized that the current code to assign constraint names to
partitions is going against the SQL standard's idea that constraint
names must be unique within a schema.  When a partition is created, the
foreign key gets exactly the same name as the constraint in the parent
table.

Now maybe you could argue that these constraints should simply be hidden
from view, because they are implementation artifacts; and then their
names don't matter.  But we already expose the partitions themselves as
individual tables, so I don't buy this argument.

One way to fix this would be to use ChooseConstraintName() for the FK in
the partition, as in the attached patch.  One caveat with this is that
there is no visual clue (in \d ) that distinguishes FKs
inherited from the parent rel from ones that have been created in the
partition directly.  I'm not sure that that's an important issue,
though.  Another point, maybe more visible, is that if you give an
explicit name to the constraint in the parent table, this is completely
lost in the partitions -- again with any visual clue to link the two.

I'm +0.2 on applying this patch to pg11, but I'd like to hear others'
opinions.

Thanks

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
diff --git a/src/backend/catalog/pg_constraint.c b/src/backend/catalog/pg_constraint.c
index f4057a9f15..9e83853dfe 100644
--- a/src/backend/catalog/pg_constraint.c
+++ b/src/backend/catalog/pg_constraint.c
@@ -490,6 +490,8 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 		ArrayType  *arr;
 		Datum		datum;
 		bool		isnull;
+		char	   *conname;
+		char	   *colname;
 
 		tuple = SearchSysCache1(CONSTROID, parentConstrOid);
 		if (!tuple)
@@ -677,8 +679,17 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 			continue;
 		}
 
+		colname = get_attname(RelationGetRelid(partRel),
+	mapped_conkey[0],
+	false);
+		conname = ChooseConstraintName(RelationGetRelationName(partRel),
+	   colname,
+	   "fkey",
+	   RelationGetNamespace(partRel),
+	   NIL);
+
 		constrOid =
-			CreateConstraintEntry(NameStr(constrForm->conname),
+			CreateConstraintEntry(conname,
   constrForm->connamespace,
   CONSTRAINT_FOREIGN,
   constrForm->condeferrable,
@@ -741,6 +752,8 @@ clone_fk_constraints(Relation pg_constraint, Relation parentRel,
 		}
 
 		ReleaseSysCache(tuple);
+		pfree(colname);
+		pfree(conname);
 	}
 
 	pfree(attmap);
diff --git a/src/test/regress/expected/foreign_key.out b/src/test/regress/expected/foreign_key.out
index 52164e89d2..aef5cc3ed0 100644
--- a/src/test/regress/expected/foreign_key.out
+++ b/src/test/regress/expected/foreign_key.out
@@ -1485,22 +1485,22 @@ INSERT INTO fk_partitioned_fk_1 (a,b) VALUES (500, 501);
 ERROR:  insert or update on table "fk_partitioned_fk_1" violates foreign key constraint "fk_partitioned_fk_a_fkey"
 DETAIL:  Key (a, b)=(500, 501) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk (a,b) VALUES (1500, 1501);
-ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_fkey"
 DETAIL:  Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk_2 (a,b) VALUES (1500, 1501);
-ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_a_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_2" violates foreign key constraint "fk_partitioned_fk_2_a_fkey"
 DETAIL:  Key (a, b)=(1500, 1501) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk (a,b) VALUES (2500, 2502);
-ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_3_1_a_fkey"
 DETAIL:  Key (a, b)=(2500, 2502) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk_3 (a,b) VALUES (2500, 2502);
-ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_a_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_3_1" violates foreign key constraint "fk_partitioned_fk_3_1_a_fkey"
 DETAIL:  Key (a, b)=(2500, 2502) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk (a,b) VALUES (2501, 2503);
-ERROR:  insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_a_fkey"
+ERROR:  insert or update on table "fk_partitioned_fk_3_0" violates foreign key constraint "fk_partitioned_fk_3_0_a_fkey"
 DETAIL:  Key (a, b)=(2501, 2503) is not present in table "fk_notpartitioned_pk".
 INSERT INTO fk_partitioned_fk_3 (a,b) 

Re: partition tree inspection functions

2018-10-12 Thread Robert Haas
On Tue, Oct 2, 2018 at 11:37 PM Michael Paquier  wrote:
> Putting the new function pg_partition_children() in partition.c is a
> bad idea I think.  So instead I think that we should put that in a
> different location, say utils/adt/partitionfuncs.c.
>
> +   TupleDescInitEntry(tupdesc, (AttrNumber) 1, "relid",
> +  REGCLASSOID, -1, 0);
> +   TupleDescInitEntry(tupdesc, (AttrNumber) 2, "parentid",
> +  REGCLASSOID, -1, 0);
> REGCLASSOID is used mainly for casting, so instead let's use OIDOID like
> any other system function.

-1.  REGCLASSOID is a lot easier for humans to read and can still be
joined to an OID column somewhere if needed.

I think we should be striving to use types like regclass more often in
system catalogs and views, not less often.

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



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Andrew Dunstan




On 10/12/2018 12:20 PM, Tom Lane wrote:

Andres Freund  writes:

I'm asking again, what exactly do we test by having these types in the
dump? They're bog standard types, there's nothing new for pg_dump to
test with them. No weird typmod rules, no weird parse-time type mapping,
nothing?

That's a pretty fair point.  The types' I/O functions will be exercised
well enough by the regression tests themselves, and it's hard to see what
more test coverage is gained by including them in pg_dump/pg_upgrade
testing.  Maybe we should just drop those tables and be done with it.



If you're happy with that then go for it. It will be less work for me ;-)

cheers

andrew


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




Re: partition tree inspection functions

2018-10-12 Thread Alvaro Herrera
On 2018-Oct-12, Robert Haas wrote:

> I think we should be striving to use types like regclass more often in
> system catalogs and views, not less often.

+1

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



Re: Why we allow CHECK constraint contradiction?

2018-10-12 Thread Robert Haas
On Wed, Oct 10, 2018 at 4:26 AM Amit Langote
 wrote:
> On the other hand, the syntax of CHECK constraints allows a fairly wide
> range of expressions to be specified, with expressions/values of arbitrary
> types and operators with arbitrary semantics (not limited to
> btree/ordering semantics, for example).  It won't be a good enough
> solution if it can provide the error for only a certain subset of
> user-specified expressions, IMHO.

It's impossible to solve that problem in general -- solving it for
only a certain subset of user expressions is the best we can ever do.
If you found a fully general solution, you would have solved the
halting problem, which has been proved to be impossible.

Now, I think there is a reasonable argument that it would still be
nice to give an ERROR diagnostic in the cases we can detect, but I do
not agree with that argument, for all of the reasons stated here: the
development resources are better spent elsewhere, somebody might be
creating such a contradictory constraint deliberately for whatever
purpose, it might annoy or confuse users to get the error in only some
cases.

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



Re: View to get all the extension control file details

2018-10-12 Thread Robert Haas
On Wed, Oct 10, 2018 at 8:27 AM Haribabu Kommi  wrote:
> Here is the patch as per the above discussion.

One potential problem with this is that we could add more control-file
attributes in the future, and it will be annoying if the view ends up
with a million columns, or if we ever have to rename them.  People who
have created objects that depend on those views may find that
pg_dump/restore or pg_upgrade fail, just as they do when we whack
around pg_stat_activity. pg_settings gets around that using an
EAV-like format.  I'm not sure that's the best solution here, but it's
something to think about.

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



Re: Why we allow CHECK constraint contradiction?

2018-10-12 Thread Tom Lane
Robert Haas  writes:
> Now, I think there is a reasonable argument that it would still be
> nice to give an ERROR diagnostic in the cases we can detect, but I do
> not agree with that argument, for all of the reasons stated here: the
> development resources are better spent elsewhere, somebody might be
> creating such a contradictory constraint deliberately for whatever
> purpose, it might annoy or confuse users to get the error in only some
> cases.

It's also arguable that throwing an ERROR would be contrary to spec,
in that it would prevent creation of constraints that the SQL standard
does not forbid.

You could dodge that problem by making the message be just a WARNING
or less.  Still, though, the other arguments-against remain valid.

regards, tom lane



Re: Requesting advanced Group By support

2018-10-12 Thread Robert Haas
On Wed, Oct 10, 2018 at 1:50 PM Tom Lane  wrote:
> It fails in cases where the data type
> considers distinguishable values to be "equal", as for example zero vs.
> minus zero in IEEE floats, or numeric values with varying numbers of
> trailing zeroes, or citext, etc.  So for example if the sno columns are
> type citext, we can be sure that a.sno does not contain both 'X' and 'x',
> because the pkey would forbid it.  But if it contains 'X', while b.sno
> contains both 'X' and 'x', then (if we allowed this case) it'd be
> indeterminate which b.sno value is returned by the GROUP BY.  One might or
> might not consider that OK for a particular application, but I don't think
> the parser should just assume for you that it is.

Since this is approximately the 437,253rd time this problem has come
up, and since even reasonably experienced hackers often get confused
about it or (ahem) momentarily forget about the problem, it is really
well paste time to find some way of labeling operator classes or
families or individual operators to indicate whether or not they are
testing precisely the exactly-the-same property.

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



AllocSetContextCreate changes breake extensions

2018-10-12 Thread Andres Freund
Hi,

Christoph Berg, on IRC, raised the issue that at least one extension
failed compiling in v11. The extension currently does:
https://github.com/pgq/pgq/blob/master/triggers/common.c#L225
tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext,
  "pgq_triggers table info",
  ALLOCSET_SMALL_MINSIZE,
  ALLOCSET_SMALL_INITSIZE,
  ALLOCSET_SMALL_MAXSIZE);

which makes sense, because it has to support versions below 9.6, which
introduced ALLOCSET_SMALL_SIZES etc.

But 9fa6f00b1308dd10da4eca2f31ccbfc7b35bb461 / Rethink MemoryContext creation 
to improve performance
causes this to fail to compile because since then AllocSetContextCreate
is declared to have three parameters:

#ifdef HAVE__BUILTIN_CONSTANT_P
#define AllocSetContextCreate(parent, name, allocparams) \
(StaticAssertExpr(__builtin_constant_p(name), \
  "memory context names must be 
constant strings"), \
 AllocSetContextCreateExtended(parent, name, allocparams))
#else
#define AllocSetContextCreate(parent, name, allocparams) \
AllocSetContextCreateExtended(parent, name, allocparams)
#endif

which means it only compiles if ALLOCSET_*_SIZES is passed, rather than
individual parameters.

I think we should fix that. If the goal was to only allow passing the
*SIZES parameters, we should have called it out as that.

Based on a quick look, ISTM the easiest fix is to have the
AllocSetContextCreate accept five parameters, and move it below the
ALLOCSET_*_SIZES macros. That way they should be expanded before
AllocSetContextCreate(), and thus 5 params should be fine.

Greetings,

Andres Freund



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Christoph Berg
Re: Andres Freund 2018-10-12 <20181012170355.bhxi273skjt6s...@alap3.anarazel.de>
> Hi,
> 
> Christoph Berg, on IRC, raised the issue that at least one extension
> failed compiling in v11. The extension currently does:
> https://github.com/pgq/pgq/blob/master/triggers/common.c#L225

Others have already been fixed, e.g. hll:

https://github.com/citusdata/postgresql-hll/pull/52/commits/e7bfbc80bbaca547167d645be11db24c8922385f

Andres' idea would enable the old code to continue to work, but
couldn't we additionally to backpatch the ALLOCSET_*_SIZES macros, so
the new code works also on old versions that didn't get the new
AllocSetContextCreate macro?

Christoph



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> Christoph Berg, on IRC, raised the issue that at least one extension
> failed compiling in v11. The extension currently does:
> https://github.com/pgq/pgq/blob/master/triggers/common.c#L225
>   tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext,
> "pgq_triggers table info",
> ALLOCSET_SMALL_MINSIZE,
> ALLOCSET_SMALL_INITSIZE,
>   ALLOCSET_SMALL_MAXSIZE);

> which makes sense, because it has to support versions below 9.6, which
> introduced ALLOCSET_SMALL_SIZES etc.

Yeah, we discussed that at the time and thought it was acceptable
collateral damage.  It's not like nobody ever breaks API in new major
versions.

> Based on a quick look, ISTM the easiest fix is to have the
> AllocSetContextCreate accept five parameters, and move it below the
> ALLOCSET_*_SIZES macros. That way they should be expanded before
> AllocSetContextCreate(), and thus 5 params should be fine.

Huh?  The order in which you #define macros doesn't affect expansion.

We could make this work more conveniently on compilers supporting
__VA_ARGS__, though.  That's certainly good enough in HEAD, and
probably good enough for most people in 11.

regards, tom lane



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Chapman Flack
On 10/12/2018 01:10 PM, Christoph Berg wrote:
> Re: Andres Freund 2018-10-12 
> Andres' idea would enable the old code to continue to work, but
> couldn't we additionally to backpatch the ALLOCSET_*_SIZES macros, so
> the new code works also on old versions that didn't get the new
> AllocSetContextCreate macro?

That's effectively what PL/Java did, just for itself. Was pretty
straightforward.

https://github.com/tada/pljava/commit/3b67999

-Chap



Re: Index Skip Scan

2018-10-12 Thread Robert Haas
On Thu, Sep 13, 2018 at 11:40 AM Jesper Pedersen
 wrote:
> > I noticed that current implementation doesn't
> > perform well when we have lots of small groups of equal values. Here is
> > the execution time of index skip scan vs unique over index scan, in ms,
> > depending on the size of group. The benchmark script is attached.
> >
> > group sizeskipunique
> > 1 2,293.85132.55
> > 5 464.40  106.59
> > 10239.61  102.02
> > 5056.59   98.74
> > 100   32.56   103.04
> > 500   6.0897.09
> >
>
> Yes, this doesn't look good. Using your test case I'm seeing that unique
> is being chosen when the group size is below 34, and skip above.

I'm not sure exactly how the current patch is approaching the problem,
but it seems like it might be reasonable to do something like -- look
for a distinct item within the current page; if not found, then search
from the root of the tree for the next item > the current item.

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



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Andres Freund
On 2018-10-12 13:20:16 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > Christoph Berg, on IRC, raised the issue that at least one extension
> > failed compiling in v11. The extension currently does:
> > https://github.com/pgq/pgq/blob/master/triggers/common.c#L225
> > tbl_cache_ctx = AllocSetContextCreate(TopMemoryContext,
> >   "pgq_triggers table info",
> >   ALLOCSET_SMALL_MINSIZE,
> >   ALLOCSET_SMALL_INITSIZE,
> >   ALLOCSET_SMALL_MAXSIZE);
> 
> > which makes sense, because it has to support versions below 9.6, which
> > introduced ALLOCSET_SMALL_SIZES etc.
> 
> Yeah, we discussed that at the time and thought it was acceptable
> collateral damage.  It's not like nobody ever breaks API in new major
> versions.

Sure, we do that all the time.  It just seems quite unnecessarily
painful here, especially because ALLOCSET_*_SIZES wasn't backpatched.


> > Based on a quick look, ISTM the easiest fix is to have the
> > AllocSetContextCreate accept five parameters, and move it below the
> > ALLOCSET_*_SIZES macros. That way they should be expanded before
> > AllocSetContextCreate(), and thus 5 params should be fine.
> 
> Huh?  The order in which you #define macros doesn't affect expansion.

return -ENOCOFFEE;


But can't we just do something like:

#if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS)
#define AllocSetContextCreate(parent, name, ...) \
(StaticAssertExpr(__builtin_constant_p(name), \
  "memory context names must be 
constant strings"), \
 AllocSetContextCreateExtended(parent, name, __VA_ARGS__))
#else
#define AllocSetContextCreate \
AllocSetContextCreateExtended
#endif

The set of compilers that have __builtin_constant_p and not vararg
macros got to be about empty.


Greetings,

Andres Freund



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> But can't we just do something like:

> #if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS)
> #define AllocSetContextCreate(parent, name, ...) \
>   (StaticAssertExpr(__builtin_constant_p(name), \
> "memory context names must be 
> constant strings"), \
>AllocSetContextCreateExtended(parent, name, __VA_ARGS__))
> #else
> #define AllocSetContextCreate \
>   AllocSetContextCreateExtended
> #endif

> The set of compilers that have __builtin_constant_p and not vararg
> macros got to be about empty.

Yeah, fair point, and anyway we don't need the StaticAssert to exist
everywhere.  I'll make it so.

Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph
suggested?  I'm not convinced of the usefulness of that, since
extensions would still have to cope with them not being present
when building against existing minor releases.

regards, tom lane



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Andres Freund
On 2018-10-12 13:51:53 -0400, Tom Lane wrote:
> Andres Freund  writes:
> > But can't we just do something like:
> 
> > #if defined(HAVE__BUILTIN_CONSTANT_P) && defined(HAVE__VA_ARGS)
> > #define AllocSetContextCreate(parent, name, ...) \
> > (StaticAssertExpr(__builtin_constant_p(name), \
> >   "memory context names must be 
> > constant strings"), \
> >  AllocSetContextCreateExtended(parent, name, __VA_ARGS__))
> > #else
> > #define AllocSetContextCreate \
> > AllocSetContextCreateExtended
> > #endif
> 
> > The set of compilers that have __builtin_constant_p and not vararg
> > macros got to be about empty.
> 
> Yeah, fair point, and anyway we don't need the StaticAssert to exist
> everywhere.  I'll make it so.

Cool.


> Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph
> suggested?  I'm not convinced of the usefulness of that, since
> extensions would still have to cope with them not being present
> when building against existing minor releases.

I'd do so. Many extensions are fine just building against a relatively
new minor release. Won't help extension authors in the next few months,
but after that...

Greetings,

Andres Freund



Re: Multi-insert into a partitioned table with before insert row trigger causes server crash on latest HEAD

2018-10-12 Thread Andres Freund
Hi,

On 2018-10-12 21:16:25 +0530, Ashutosh Sharma wrote:
> The copy command on partitioned table causes server crash when before
> insert row trigger is created on one of its partition. Please find the
> following test-case  to reproduce the crash.
> 
> -- create a partitioned table
> create table part_copy_test  (a int, b int, c text) partition by list (b);
> create table part_copy_test_a1 partition of part_copy_test for values in(1);
> create table part_copy_test_a2 partition of part_copy_test for values in(2);
> 
> -- insert enough rows to allow multi-insert into a partitioned table.
> copy (select x,1,'One' from generate_series(1,1000) x
> union all
> select x,2,'two' from generate_series(1001,1010) x
> union all
> select x,1,'One' from generate_series(1011,1020) x) to
> '/tmp/multi_insert_data.csv';
> 
> -- create before insert row trigger on part_copy_test_a2
> create function part_ins_func() returns trigger language plpgsql as
> $$
> begin
>   return new;
> end;
> $$;
> 
> create trigger part_ins_trig before insert on part_copy_test_a2
> for each row execute procedure part_ins_func();
> 
> -- run copy command on partitioned table.
> copy part_copy_test from '/tmp/multi_insert_data.csv';
> 
> postgres=# copy part_copy_test from '/tmp/multi_insert_data.csv';
> server closed the connection unexpectedly
> This probably means the server terminated abnormally
> before or while processing the request.
> The connection to the server was lost. Attempting reset: Failed.
> !>
> 
> I've spent some time looking into this issue and found that,
> CopyFrom() is trying perform multi-insert for the partition that has
> before insert row trigger created on it which is not expected. When a
> normal table with before insert row trigger is created, CopyFrom
> doesn't allow multi insert on such tables and i guess same should be
> the case with table partition as well. Please correct me if i my
> understanding is wrong ?

Yea, that anlysis sounds right. Peter?

Greetings,

Andres Freund



CopyFrom() has become way too complicated

2018-10-12 Thread Andres Freund
Hi,

I've had to whack CopyFrom() around a couple times due to the pluggable
storage patch, and I found that after
commit 0d5f05cde011512e605bb2688d9b1fbb5b3ae152
Author: Peter Eisentraut 
Date:   2018-08-01 10:23:09 +0200

Allow multi-inserts during COPY into a partitioned table

and also the previous partition support changes, the CopyFrom() code
has, for me at least, become very hard to follow.

I think the code needs to be split up so that CopyFrom() in the loop
body calls CopyFromOneTuple(), which then also splits out the tuple
routing into its own CopyFromOneTupleRoute() function (that's 200 LOC on
its own...). I suspect it'd also be good to refactor the
partition-change code out into its own function.

Greetings,

Andres Freund



Re: AllocSetContextCreate changes breake extensions

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> On 2018-10-12 13:51:53 -0400, Tom Lane wrote:
>> Shall we also backpatch the ALLOCSET_*_SIZES macros as Christoph
>> suggested?  I'm not convinced of the usefulness of that, since
>> extensions would still have to cope with them not being present
>> when building against existing minor releases.

> I'd do so. Many extensions are fine just building against a relatively
> new minor release. Won't help extension authors in the next few months,
> but after that...

I'm still not very convinced, but it's easy and harmless, so done.

regards, tom lane



Re: pgbench exit code

2018-10-12 Thread Fabien COELHO


Hello Peter,


The abort is by a client, but the code seems to only check the first
client of a thread. ISTM that if the second or later client abort it may
not be detected? Probably an intermediate aggregation at the thread level
is needed, or maybe a global variable, or as errors are counted somewhere,
it may be enough just to check that the count is non zero?


fixed


With an aggregation. Fine with me.


The patch removes a check that there was an output and that no
transactions where processed. ISTM it should be kept. If a different exit
status is chosen on abort, that would allow to keep it easily.


fixed


Ok.


Probably there should be some documentation changes as well?


done


Ok.

Patch applies cleanly, compiles, global & local "make check" are ok.
Doc build is okay.

I noticed "for (int i"… pgbench is welcome to C99:-)

No further remarks. I turned the patch as ready on the CF app.

Attached a file I used for some manual tests.

--
Fabien.

bad-sql-proba.sql
Description: application/sql


Re: Make Windows print float exponents like everybody else?

2018-10-12 Thread Tom Lane
Andres Freund  writes:
> On 2018-10-09 18:00:54 -0400, Tom Lane wrote:
>>> Also, we have quite a few variant expected-files that exist only to cater
>>> for Windows' habit of printing three exponent digits where everybody else
>>> prints just two.  It struck me that it would not be hard, or expensive,
>>> to undo that choice in snprintf.c (see attached untested patch).  So we
>>> could considerably reduce future maintenance pain for the affected tests
>>> by getting rid of those files.

> No pushback here. Lower likelihood of turning the buildfarm red, lower
> likelihood of being embarrased. Good. ;)

Some (but curiously, not all) of the Windows critters are unhappy with
the fact that I removed the extra expected files for the ECPG tests.
In retrospect, the reason is obvious: the ECPG test programs do not
include port.h nor link with src/common, so they're not using our
custom printf, just the platform native one.

I could just revert the ECPG aspects of that commit, but considering
that it seems like everything else works, it's annoying to still need
extra expected files for ECPG.

What I'm now thinking about is modifying the three affected test
programs so that they internally strip the extra zero using the
same logic as in snprintf.c.  It'd be slightly more work but
might be worth it in the long run.  On the other hand, we hardly
ever touch the ECPG tests anyway, so maybe it isn't?

regards, tom lane



Re: Maximum password length

2018-10-12 Thread Stephen Frost
Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> I've attached 2 patches in an effort to clarify the upper bounds on
> password lengths:
> - 0001 refactors the hard-coded 100 character buffer size used for
>   password prompts for client utilities into a
>   PROMPT_MAX_PASSWORD_LENGTH macro in postgres_fe.h.
> - 0002 is an attempt at documenting the password length
>   restrictions and suggested workarounds for longer passwords.

If we're going to do work in this area, why wouldn't we have the client
tools and the server agree on the max length and then have them all be
consistent..?

Seems odd to decide that 100 character buffer size in the clients makes
sense and then make the server support an 8k password.

I'm also trying to figure out why it makes sense to support an 8k
password and if we've really tried seeing what happens if pg_authid gets
a toast table that's actually used for passwords...

I'll note your patches neglected to include any tests...

Thanks!

Stephen


signature.asc
Description: PGP signature


FULL JOIN planner deficiency

2018-10-12 Thread Tom Lane
Consider this simple query:

regression=# explain select * from
int8_tbl as a1 full join (select 1 as id) as a2 on (a1.q1 = a2.id);
QUERY PLAN
--
 Hash Full Join  (cost=0.03..1.11 rows=5 width=20)
   Hash Cond: (a1.q1 = (1))
   ->  Seq Scan on int8_tbl a1  (cost=0.00..1.05 rows=5 width=16)
   ->  Hash  (cost=0.02..0.02 rows=1 width=4)
 ->  Result  (cost=0.00..0.01 rows=1 width=4)
(5 rows)

Not too exciting-looking.  But this ought to be exactly equivalent:

regression=# create table dual();
CREATE TABLE
regression=# insert into dual default values;
INSERT 0 1
regression=# explain select * from
int8_tbl as a1 full join (select 1 as id from dual) as a2 on (a1.q1 = 
a2.id);
ERROR:  FULL JOIN is only supported with merge-joinable or hash-joinable join 
conditions

I ran into this while testing the patch mentioned in
<5395.1539275...@sss.pgh.pa.us>, which basically causes the FROM-less
subselect to be treated the same as the "FROM dual" case.  But it's
a pre-existing, and long-standing, problem.

The root of the problem is that once the constant "1" has been pulled
up from the sub-select, we have a join qual that looks like "a1.q1 = 1",
and that is not a mergeable or hashable join qual, because it fails to
compare expressions from the two sides of the join.

I spent awhile thinking about whether we could generalize our notion
of mergeability, or hashability, to make this work, but it soon made
my head hurt.  Even if it's possible it would likely not be a change
we'd want to backpatch.

However, there's another way to deal with it, which is to wrap the
pulled-up constant in a PlaceHolderVar, which will cause it to act
like a Var for the purpose of recognizing a qual as mergeable/hashable.
The attached two-line (sans tests) patch does this and fixes the problem.

While this could in theory reduce our ability to optimize things
(by making expressions look unequal that formerly looked equal),
I do not think it's a big problem because our ability to optimize
full joins is pretty darn limited anyway.

Given the lack of complaints, I'm not real sure whether this is
worth back-patching.  Thoughts?

regards, tom lane

diff --git a/src/backend/optimizer/prep/prepjointree.c b/src/backend/optimizer/prep/prepjointree.c
index 688b3a1..cd6e119 100644
*** a/src/backend/optimizer/prep/prepjointree.c
--- b/src/backend/optimizer/prep/prepjointree.c
*** replace_vars_in_jointree(Node *jtnode,
*** 2044,2049 
--- 2044,2061 
  		}
  		replace_vars_in_jointree(j->larg, context, lowest_nulling_outer_join);
  		replace_vars_in_jointree(j->rarg, context, lowest_nulling_outer_join);
+ 
+ 		/*
+ 		 * Use PHVs within the join quals of a full join, even when it's the
+ 		 * lowest nulling outer join.  Otherwise, we cannot identify which
+ 		 * side of the join a pulled-up var-free expression came from, which
+ 		 * can lead to failure to make a plan at all because none of the quals
+ 		 * appear to be mergeable or hashable conditions.  For this purpose we
+ 		 * don't care about the state of wrap_non_vars, so leave it alone.
+ 		 */
+ 		if (j->jointype == JOIN_FULL)
+ 			context->need_phvs = true;
+ 
  		j->quals = pullup_replace_vars(j->quals, context);
  
  		/*


Re: Maximum password length

2018-10-12 Thread Isaac Morland
On Fri, 12 Oct 2018 at 16:52, Stephen Frost  wrote:


> I'm also trying to figure out why it makes sense to support an 8k
> password and if we've really tried seeing what happens if pg_authid gets
> a toast table that's actually used for passwords...
>

pg_authid.rolpassword stores a hash, so the password length does not affect
it.

Of course, this also means that even in principle super-long passwords
don't increase security, since one "can" (again, in principle) brute-force
any password by guessing the first
not-very-many-more-than-the-total-number-of-distinct-hashes possible
passwords, starting with the shortest passwords and working up to longer
passwords.

It's also obvious that past a certain point, longer passwords don't help
anyway, because it's already enough to have a password that can't be
guessed in, say, the expected duration of the Earth's existence using all
the computing power currently available in the world.

I agree there should be a specific limit that is the same in libpq, on the
server, and in the protocol. Maybe 128 characters, to get a nice round
number? This is still way longer than the 32-byte SHA 256 hash. Or 64,
which is still plenty but doesn't involve extending the current character
buffer size to a longer value while still hugely exceeding the amount of
information in the hash.


Re: Maximum password length

2018-10-12 Thread Bossart, Nathan
Hi Stephen,

On 10/12/18, 3:52 PM, "Stephen Frost"  wrote:
> If we're going to do work in this area, why wouldn't we have the client
> tools and the server agree on the max length and then have them all be
> consistent..?
> 
> Seems odd to decide that 100 character buffer size in the clients makes
> sense and then make the server support an 8k password.

I considered this but wondered if expanding the buffers over 80x was
too intrusive or if the 100 character limit had some historical
purpose.  I'm happy to align everything if desired.

> I'm also trying to figure out why it makes sense to support an 8k
> password and if we've really tried seeing what happens if pg_authid gets
> a toast table that's actually used for passwords...

Since v10+ always stores passwords encrypted [0], I don't think it
will require a TOAST table.

> I'll note your patches neglected to include any tests...

I will look into adding tests.  I've also been told that there may be
some limits for the .pgpass file, so I am looking into that as well.

Nathan

[0] 
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=eb61136dc75a76caef8460fa939244d8593100f2



Re: Maximum password length

2018-10-12 Thread Stephen Frost
Greetings,

* Isaac Morland (isaac.morl...@gmail.com) wrote:
> On Fri, 12 Oct 2018 at 16:52, Stephen Frost  wrote:
> > I'm also trying to figure out why it makes sense to support an 8k
> > password and if we've really tried seeing what happens if pg_authid gets
> > a toast table that's actually used for passwords...
> 
> pg_authid.rolpassword stores a hash, so the password length does not affect
> it.

I had been thinking about storing of plaintext passwords, which we
certainly used to do, but forgot that we actually did remove that,
finally, so this specific point isn't a concern any longer, though of
course the rest is.

> Of course, this also means that even in principle super-long passwords
> don't increase security, since one "can" (again, in principle) brute-force
> any password by guessing the first
> not-very-many-more-than-the-total-number-of-distinct-hashes possible
> passwords, starting with the shortest passwords and working up to longer
> passwords.

Well, as you say, length doesn't matter here, if all you're doing is
enumerating all possible responses to the server.

> It's also obvious that past a certain point, longer passwords don't help
> anyway, because it's already enough to have a password that can't be
> guessed in, say, the expected duration of the Earth's existence using all
> the computing power currently available in the world.

Not sure I really am all that keen to get into that debate. :)

> I agree there should be a specific limit that is the same in libpq, on the
> server, and in the protocol. Maybe 128 characters, to get a nice round
> number? This is still way longer than the 32-byte SHA 256 hash. Or 64,
> which is still plenty but doesn't involve extending the current character
> buffer size to a longer value while still hugely exceeding the amount of
> information in the hash.

I certainly don't think that we should break things which do work today,
which would include long plaintext passwords sent by clients.

Even if our clients don't support >100 character passwords, if the
server does, then someone might be using one.

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: Maximum password length

2018-10-12 Thread Stephen Frost
Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 10/12/18, 3:52 PM, "Stephen Frost"  wrote:
> > If we're going to do work in this area, why wouldn't we have the client
> > tools and the server agree on the max length and then have them all be
> > consistent..?
> > 
> > Seems odd to decide that 100 character buffer size in the clients makes
> > sense and then make the server support an 8k password.
> 
> I considered this but wondered if expanding the buffers over 80x was
> too intrusive or if the 100 character limit had some historical
> purpose.  I'm happy to align everything if desired.

The way to sort that out would likely to be go look at the history...

That said, assuming we do adjust the limit to be higher, it'd probably
make more sense to allocate it and not just have it on the stack (which
might be why it's the size it is today...).

> > I'm also trying to figure out why it makes sense to support an 8k
> > password and if we've really tried seeing what happens if pg_authid gets
> > a toast table that's actually used for passwords...
> 
> Since v10+ always stores passwords encrypted [0], I don't think it
> will require a TOAST table.

Yeah, that was pointed out downthread, I'd forgotten that we (finally)
got rid of storing plaintext passwords; sometimes it's difficult to
believe that we've actually moved forward with something that some of us
complained about many, many, many years ago. ;)

> > I'll note your patches neglected to include any tests...
> 
> I will look into adding tests.  I've also been told that there may be
> some limits for the .pgpass file, so I am looking into that as well.

Ok.

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: Maximum password length

2018-10-12 Thread Tom Lane
Isaac Morland  writes:
> On Fri, 12 Oct 2018 at 16:52, Stephen Frost  wrote:
>> I'm also trying to figure out why it makes sense to support an 8k
>> password and if we've really tried seeing what happens if pg_authid gets
>> a toast table that's actually used for passwords...

> ...
> It's also obvious that past a certain point, longer passwords don't help
> anyway, because it's already enough to have a password that can't be
> guessed in, say, the expected duration of the Earth's existence using all
> the computing power currently available in the world.

And, of course, who is really going to type a password longer than a
couple dozen characters?  And get it right reliably, when they can't
see what they're typing?  But even if you assume the password is never
manually entered but just lives in somebody's .pgpass, it's pointless
to make it so long.  Then the attacker will just switch to brute-forcing
the user's login password, or whereever along the chain there actually
is a manually-entered password.

I concur that we might as well standardize on something in the range
of 64 to 100 characters.  1K is silly, even if somewhere there is a
spec that allows it.

regards, tom lane



Re: Maximum password length

2018-10-12 Thread Bossart, Nathan
Hi Isaac,

On 10/12/18, 4:04 PM, "Isaac Morland"  wrote:
> I agree there should be a specific limit that is the same in libpq,
> on the server, and in the protocol. Maybe 128 characters, to get a
> nice round number? This is still way longer than the 32-byte SHA 256
> hash. Or 64, which is still plenty but doesn't involve extending the
> current character buffer size to a longer value while still hugely
> exceeding the amount of information in the hash.

My main motivation for suggesting the increase to 8k is to provide
flexibility for alternative authentication methods like LDAP, RADIUS,
PAM, and BSD.

Nathan



Re: Maximum password length

2018-10-12 Thread Stephen Frost
Greetings,

* Bossart, Nathan (bossa...@amazon.com) wrote:
> On 10/12/18, 4:04 PM, "Isaac Morland"  wrote:
> > I agree there should be a specific limit that is the same in libpq,
> > on the server, and in the protocol. Maybe 128 characters, to get a
> > nice round number? This is still way longer than the 32-byte SHA 256
> > hash. Or 64, which is still plenty but doesn't involve extending the
> > current character buffer size to a longer value while still hugely
> > exceeding the amount of information in the hash.
> 
> My main motivation for suggesting the increase to 8k is to provide
> flexibility for alternative authentication methods like LDAP, RADIUS,
> PAM, and BSD.

Specific use-cases here would be better than hand-waving at "these other
things."  Last I checked, all of those work with what we've got today
and I don't recall hearing complaints about them not working due to this
limit.

Thanks!

Stephen


signature.asc
Description: PGP signature


Re: TupleTableSlot abstraction

2018-10-12 Thread Andres Freund
On 2018-10-09 20:46:04 +0530, Amit Khandekar wrote:
> On Wed, 26 Sep 2018 at 05:35, Andres Freund  wrote:
> > > +
> > > +/*
> > > + * This is a function used by all getattr() callbacks which deal with a 
> > > heap
> > > + * tuple or some tuple format which can be represented as a heap tuple 
> > > e.g. a
> > > + * minimal tuple.
> > > + *
> > > + * heap_getattr considers any attnum beyond the attributes available in 
> > > the
> > > + * tuple as NULL. This function however returns the values of missing
> > > + * attributes from the tuple descriptor in that case. Also this function 
> > > does
> > > + * not support extracting system attributes.
> > > + *
> > > + * If the attribute needs to be fetched from the tuple, the function 
> > > fills in
> > > + * tts_values and tts_isnull arrays upto the required attnum.
> > > + */
> > > +Datum
> > > +tts_heap_getattr_common(TupleTableSlot *slot, HeapTuple tuple, uint32 
> > > *offp,
> > > + int attnum, bool
> > > *isnull)
> >
> > I'm still *vehemently* opposed to the introduction of this.
> 
> You mean, you want to remove the att_isnull() optimization, right ?

Yes.


> Removed that code now. Directly deforming the tuple regardless of the
> null attribute.

Good, thanks.


> > > @@ -2024,7 +2024,18 @@ FormIndexDatum(IndexInfo *indexInfo,
> > >   Datum   iDatum;
> > >   boolisNull;
> > >
> > > - if (keycol != 0)
> > > + if (keycol < 0)
> > > + {
> > > + HeapTupleTableSlot *hslot = (HeapTupleTableSlot 
> > > *)slot;
> > > +
> > > + /* Only heap tuples have system attributes. */
> > > + Assert(TTS_IS_HEAPTUPLE(slot) || 
> > > TTS_IS_BUFFERTUPLE(slot));
> > > +
> > > + iDatum = heap_getsysattr(hslot->tuple, keycol,
> > > +  
> > > slot->tts_tupleDescriptor,
> > > +  
> > > &isNull);
> > > + }
> > > + else if (keycol != 0)
> > >   {
> > >   /*
> > >* Plain index column; get the value we need 
> > > directly from the
> >
> > This now should access the system column via the slot, right?  There's
> > other places like this IIRC.
> 
> Done. In FormIndexDatum() and ExecInterpExpr(), directly calling
> slot_getsysattr() now.
> 
> In ExecInterpExpr (), I am no longer using ExecEvalSysVar() now. I am
> planning to remove this definition since it would be a single line
> function just calling slot_getsysattr().
> 
> In build_ExecEvalSysVar(), ExecEvalSysVar() is still used, so I
> haven't removed the definition yet. I am planning to create a new
> LLVMValueRef FuncSlotGetsysattr, and use that instead, in
> build_ExecEvalSysVar(), or for that matter, I am thinking to revert
> back build_ExecEvalSysVar() and instead have that code inline as in
> HEAD.

I'd just have ExecInterpExpr() continue calling ExecEvalSysVar. There's
no reason for it to be inline. And it's simpler for JIT than the
alternative.

> > > @@ -185,6 +1020,7 @@ ExecResetTupleTable(List *tupleTable,/* tuple 
> > > table */
> > >   {
> > >   TupleTableSlot *slot = lfirst_node(TupleTableSlot, lc);
> > >
> > > + slot->tts_cb->release(slot);
> > >   /* Always release resources and reset the slot to empty */
> > >   ExecClearTuple(slot);
> > >   if (slot->tts_tupleDescriptor)
> > > @@ -240,6 +1076,7 @@ void
> > >  ExecDropSingleTupleTableSlot(TupleTableSlot *slot)
> > >  {
> > >   /* This should match ExecResetTupleTable's processing of one slot */
> > > + slot->tts_cb->release(slot);
> > >   Assert(IsA(slot, TupleTableSlot));
> > >   ExecClearTuple(slot);
> > >   if (slot->tts_tupleDescriptor)
> >
> > ISTM that release should be called *after* clearing the slot.
> 
> I am copying here what I discussed about this in the earlier reply:
> 
> I am not sure what was release() designed to do. Currently all of the
> implementers of this function are empty.

So additional deallocations can happen in a slot. We might need this
e.g. at some point for zheap which needs larger, longer-lived, buffers
in slots.

> Was it meant for doing
> ReleaseTupleDesc(slot->tts_tupleDescriptor) ? Or
> ReleaseBuffer(bslot->buffer) ?

No. The former lives in generic code, the latter is in ClearTuple.

> I think the purpose of keeping this *before* clearing the tuple might
> be because the clear() might have already cleared some handles that
> release() might need.

It's just plainly wrong to call it this way round.


> I went ahead and did these changes, but for now, I haven't replaced
> ExecFetchSlotTuple() with ExecFetchSlotHeapTuple(). Instead, I
> retained ExecFetchSlotTuple() to be called for heap tuples, and added
> a new ExecFetchGenericSlotT

Re: Calculate total_table_pages after set_base_rel_sizes()

2018-10-12 Thread David Rowley
On 12 October 2018 at 23:35, Amit Langote  wrote:
> On 2018/10/11 13:45, Amit Langote wrote:
>> On 2018/10/07 17:43, David Rowley wrote:
>>> Amit Langote has since posted a patch to delay the RangeTblEntry
>>> creation until after pruning. His patch happens to also move the
>>> total_table_pages calculation, but I believe this change should be
>>> made as an independent commit to anything else.  I've kept it in the
>>> commitfest for that reason.
>>
>> Yeah, if this patch is a win independent of the other project of delaying
>> partition RTE creation, which seems to be true, I think we should go ahead
>> with applying this patch.
>
> Fwiw, I've incorporated David's patch in my own series, so that one of my
> patches no longer has the code movement hunks that are in his.  I will
> post the new version of my patch series like that.

Thanks. I'll keep this open here anyway so the change can be
considered independently. I think counting pages of pruned partitions
is a bug that should be fixed. You can just drop that patch from your
set if this gets committed.


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



Re: Calculate total_table_pages after set_base_rel_sizes()

2018-10-12 Thread Amit Langote
On Sat, Oct 13, 2018 at 7:36 David Rowley 
wrote:

> On 12 October 2018 at 23:35, Amit Langote 
> wrote:
> > On 2018/10/11 13:45, Amit Langote wrote:
> >> On 2018/10/07 17:43, David Rowley wrote:
> >>> Amit Langote has since posted a patch to delay the RangeTblEntry
> >>> creation until after pruning. His patch happens to also move the
> >>> total_table_pages calculation, but I believe this change should be
> >>> made as an independent commit to anything else.  I've kept it in the
> >>> commitfest for that reason.
> >>
> >> Yeah, if this patch is a win independent of the other project of
> delaying
> >> partition RTE creation, which seems to be true, I think we should go
> ahead
> >> with applying this patch.
> >
> > Fwiw, I've incorporated David's patch in my own series, so that one of my
> > patches no longer has the code movement hunks that are in his.  I will
> > post the new version of my patch series like that.
>
> Thanks. I'll keep this open here anyway so the change can be
> considered independently. I think counting pages of pruned partitions
> is a bug that should be fixed. You can just drop that patch from your
> set if this gets committed.


Yeah, that was my plan anyway.

Thanks,
Amit

>
>


Re: Maximum password length

2018-10-12 Thread Bossart, Nathan
On 10/12/18, 4:24 PM, "Stephen Frost"  wrote:
> * Bossart, Nathan (bossa...@amazon.com) wrote:
>> My main motivation for suggesting the increase to 8k is to provide
>> flexibility for alternative authentication methods like LDAP, RADIUS,
>> PAM, and BSD.
>
> Specific use-cases here would be better than hand-waving at "these other
> things."  Last I checked, all of those work with what we've got today
> and I don't recall hearing complaints about them not working due to this
> limit.

The main one I am thinking of is generated security tokens.  It seems
reasonable to me to limit md5 and scram-sha-256 passwords to a much
shorter length, but I think the actual server message limit should be
somewhat more flexible.

Nathan



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Tom Lane
Andrew Dunstan  writes:
> On 10/12/2018 12:20 PM, Tom Lane wrote:
>> That's a pretty fair point.  The types' I/O functions will be exercised
>> well enough by the regression tests themselves, and it's hard to see what
>> more test coverage is gained by including them in pg_dump/pg_upgrade
>> testing.  Maybe we should just drop those tables and be done with it.

> If you're happy with that then go for it. It will be less work for me ;-)

Done.

regards, tom lane



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Tom Lane
BTW, I was annoyed while looking things over that this patch had broken
a couple of comments in opr_sanity.sql:

@@ -823,7 +823,6 @@ WHERE a.aggfnoid = p.oid AND
 
 -- Cross-check transfn against its entry in pg_proc.
 -- NOTE: use physically_coercible here, not binary_coercible, because
--- max and min on abstime are implemented using int4larger/int4smaller.
 SELECT a.aggfnoid::oid, p.proname, ptr.oid, ptr.proname
 FROM pg_aggregate AS a, pg_proc AS p, pg_proc AS ptr
 WHERE a.aggfnoid = p.oid AND
@@ -978,7 +977,6 @@ WHERE a.aggfnoid = p.oid AND
 -- Check that all combine functions have signature
 -- combine(transtype, transtype) returns transtype
 -- NOTE: use physically_coercible here, not binary_coercible, because
--- max and min on abstime are implemented using int4larger/int4smaller.
 
 SELECT a.aggfnoid, p.proname
 FROM pg_aggregate as a, pg_proc as p

Just removing a fraction of the sentence is not good.

So I went looking for a different example to plug in there, and soon
found that there weren't any.  If you change all the physically_coercible
calls in that script to binary_coercible, its output doesn't change.

I'm thinking that we ought to do that, and just get rid of
physically_coercible(), so that we have a tighter, more semantically
meaningful set of checks here.  We can always undo that if we ever
have occasion to type-cheat like that again, but offhand I'm not sure
why we would do so.

regards, tom lane



Re: Maximum password length

2018-10-12 Thread Tom Lane
"Bossart, Nathan"  writes:
> On 10/12/18, 4:24 PM, "Stephen Frost"  wrote:
>> Specific use-cases here would be better than hand-waving at "these other
>> things."  Last I checked, all of those work with what we've got today
>> and I don't recall hearing complaints about them not working due to this
>> limit.

> The main one I am thinking of is generated security tokens.  It seems
> reasonable to me to limit md5 and scram-sha-256 passwords to a much
> shorter length, but I think the actual server message limit should be
> somewhat more flexible.

Sure, but even a generated security token seems unlikely to be more
than a couple dozen bytes long.  What's the actual use-case for tokens
longer than that?  ISTM that a limit around 100 bytes already has a
whole lot of headroom.

regards, tom lane



Re: [HACKERS] removing abstime, reltime, tinterval.c, spi/timetravel

2018-10-12 Thread Andres Freund
Hi,

On 2018-10-12 19:47:40 -0400, Tom Lane wrote:
> BTW, I was annoyed while looking things over that this patch had broken
> a couple of comments in opr_sanity.sql:
> 
> @@ -823,7 +823,6 @@ WHERE a.aggfnoid = p.oid AND
>  
>  -- Cross-check transfn against its entry in pg_proc.
>  -- NOTE: use physically_coercible here, not binary_coercible, because
> --- max and min on abstime are implemented using int4larger/int4smaller.
>  SELECT a.aggfnoid::oid, p.proname, ptr.oid, ptr.proname
>  FROM pg_aggregate AS a, pg_proc AS p, pg_proc AS ptr
>  WHERE a.aggfnoid = p.oid AND
> @@ -978,7 +977,6 @@ WHERE a.aggfnoid = p.oid AND
>  -- Check that all combine functions have signature
>  -- combine(transtype, transtype) returns transtype
>  -- NOTE: use physically_coercible here, not binary_coercible, because
> --- max and min on abstime are implemented using int4larger/int4smaller.
>  
>  SELECT a.aggfnoid, p.proname
>  FROM pg_aggregate as a, pg_proc as p
> 
> Just removing a fraction of the sentence is not good.

Fair.


> So I went looking for a different example to plug in there, and soon
> found that there weren't any.  If you change all the physically_coercible
> calls in that script to binary_coercible, its output doesn't change.

> I'm thinking that we ought to do that, and just get rid of
> physically_coercible(), so that we have a tighter, more semantically
> meaningful set of checks here.  We can always undo that if we ever
> have occasion to type-cheat like that again, but offhand I'm not sure
> why we would do so.

Hm, I wonder if it's not a good idea to leave the test there, or rewrite
it slightly, so we have a a more precise warning about cheats like that?

I also don't really see why we'd introduce new hacks like reusing
functions like that, but somebody might do it while hacking something
together and forget about it.

Probably still not worth it?

Greetings,

Andres Freund



Re: Alter index rename concurrently to

2018-10-12 Thread Andres Freund
Hi,

On 2018-10-05 12:03:54 +0200, Peter Eisentraut wrote:
> From 37591a06901e2d694e3928b7e1cddfcfd84f6267 Mon Sep 17 00:00:00 2001
> From: Peter Eisentraut 
> Date: Mon, 13 Aug 2018 22:38:36 +0200
> Subject: [PATCH v2] Lower lock level for renaming indexes
> 
> Change lock level for renaming index (either ALTER INDEX or implicitly
> via some other commands) from AccessExclusiveLock to
> ShareUpdateExclusiveLock.
> 
> The reason we need a strong lock at all for relation renaming is that
> the name change causes a rebuild of the relcache entry.  Concurrent
> sessions that have the relation open might not be able to handle the
> relcache entry changing underneath them.  Therefore, we need to lock the
> relation in a way that no one can have the relation open concurrently.
> But for indexes, the relcache handles reloads specially in
> RelationReloadIndexInfo() in a way that keeps changes in the relcache
> entry to a minimum.  As long as no one keeps pointers to rd_amcache and
> rd_options around across possible relcache flushes, which is the case,
> this ought to be safe.
> 
> One could perhaps argue that this could all be done with an
> AccessShareLock then.  But we standardize here for consistency on
> ShareUpdateExclusiveLock, which is already used by other DDL commands
> that want to operate in a concurrent manner.  For example, renaming an
> index concurrently with CREATE INDEX CONCURRENTLY might be trouble.

I don't see how this could be argued. It has to be a self-conflicting
lockmode, otherwise you'd end up doing renames of tables where you
cannot see the previous state. And you'd get weird errors about updating
invisible rows etc.


> @@ -3155,11 +3157,13 @@ RenameRelationInternal(Oid myrelid, const char 
> *newrelname, bool is_internal)
>   Oid namespaceId;
>  
>   /*
> -  * Grab an exclusive lock on the target table, index, sequence, view,
> -  * materialized view, or foreign table, which we will NOT release until
> -  * end of transaction.
> +  * Grab a lock on the target relation, which we will NOT release until 
> end
> +  * of transaction.  The lock here guards mainly against triggering
> +  * relcache reloads in concurrent sessions, which might not handle this
> +  * information changing under them.  For indexes, we can use a reduced
> +  * lock level because RelationReloadIndexInfo() handles indexes 
> specially.
>*/

I don't buy this description. Imo it's a fundamental correctness
thing. Without it concurrent DDL would potentially overwrite the rename,
because they could start updating while still seeing the old version.


Greetings,

Andres Freund



Re: Maximum password length

2018-10-12 Thread Bossart, Nathan
On 10/12/18, 7:02 PM, "Tom Lane"  wrote:
> "Bossart, Nathan"  writes:
>> The main one I am thinking of is generated security tokens.  It seems
>> reasonable to me to limit md5 and scram-sha-256 passwords to a much
>> shorter length, but I think the actual server message limit should be
>> somewhat more flexible.
>
> Sure, but even a generated security token seems unlikely to be more
> than a couple dozen bytes long.  What's the actual use-case for tokens
> longer than that?  ISTM that a limit around 100 bytes already has a
> whole lot of headroom.

I can't speak to the technical necessity of longer tokens, but several
services provide them.  One specific example is the AWS Security Token
Service.  The documentation for that service currently suggests that
"the typical size is less than 4096 bytes..." [0].  I understand this
alone doesn't warrant a change to PostgreSQL, but it seems valuable to
me to ease this restriction on custom client authentication
mechanisms.

Regarding md5 and scram-sha-256 passwords, I'll look into establishing
some sort of maximum password length that is well-documented and
provides users with clear error messages.  My vote would be something
like 128 characters just to be safe.  One interesting question is how
we handle existing longer passwords after upgrading.  Maybe we could
continue to allow longer passwords to be used for authentication and
only restrict the length of new ones.

Nathan

[0] https://docs.aws.amazon.com/STS/latest/APIReference/API_GetSessionToken.html