Re: Debian mips: Failed test 'Check expected t_009_tbl data on standby'
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
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
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
> 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
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()
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
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
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
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
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
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
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
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
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
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
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
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
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
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
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?
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
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?
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
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
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
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
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
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
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
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
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
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
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
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
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
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?
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
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
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
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
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
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
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
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
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
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
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()
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()
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
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
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
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
"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
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
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
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
