Re: Server crash on RHEL 9/s390x platform against PG16
On Sat, Oct 21, 2023 at 5:17 AM Andres Freund wrote: > Hi, > > On 2023-09-12 15:27:21 +0530, Suraj Kharage wrote: > > *[edb@9428da9d2137 postgres]$ cat /etc/redhat-release AlmaLinux release > 9.2 > > (Turquoise Kodkod)[edb@9428da9d2137 postgres]$ lscpuArchitecture: > > s390x CPU op-mode(s): 32-bit, 64-bit Address sizes:39 > bits > > Can you provide the rest of the lscpu output? There have been issues with > Z14 > vs Z15: > https://github.com/llvm/llvm-project/issues/53009 > > You're apparently not hitting that, but given that fact, you either are on > a > slightly older CPU, or you have applied a patch to work around it. Because > otherwise your uild instructions below would hit that problem, I think. > > > > physical, 48 bits virtual Byte Order: Big Endian* > > *Configure command:* > > ./configure --prefix=/home/edb/postgres/ --with-lz4 --with-zstd > --with-llvm > > --with-perl --with-python --with-tcl --with-openssl --enable-nls > > --with-libxml --with-libxslt --with-systemd --with-libcurl --without-icu > > --enable-debug --enable-cassert --with-pgport=5414 > > Hm, based on "--with-libcurl" this isn't upstream postgres, correct? Have > you > verified the issue reproduces on upstream postgres? > Yes, I can reproduce this on upstream postgres master and v16 branch. Here are details: ./configure --prefix=/home/edb/postgres/ --with-zstd --with-llvm --with-perl --with-python --with-tcl --with-openssl --enable-nls --with-libxml --with-libxslt --with-systemd --without-icu --enable-debug --enable-cassert --with-pgport=5414 CFLAGS="-g -O0" [edb@9428da9d2137 postgres]$ cat /etc/redhat-release AlmaLinux release 9.2 (Turquoise Kodkod) [edb@9428da9d2137 edbas]$ lscpu Architecture: s390x CPU op-mode(s): 32-bit, 64-bit Address sizes:39 bits physical, 48 bits virtual Byte Order: Big Endian CPU(s): 9 On-line CPU(s) list: 0-8 Vendor ID: GenuineIntel Model name: Intel(R) Core(TM) i7-9750H CPU @ 2.60GHz CPU family: 6 Model: 158 Thread(s) per core: 1 Core(s) per socket: 1 Socket(s): 9 Stepping: 10 BogoMIPS: 5200.00 Flags: fpu vme de pse tsc msr pae mce cx8 apic sep mtrr pge mca cmov pat pse36 clflush mmx fxsr sse sse2 ss ht pbe syscall nx pdpe1gb lm constant_tsc rep_good nopl xtopology nonstop_tsc cpuid pni pclmulqdq dtes64 ds_cpl ssse3 sdbg fma cx 16 xtpr pcid sse4_1 sse4_2 movbe popcnt aes xsave avx f16c rdrand hypervisor lahf_lm abm 3dnowprefetch fsgsbase bmi1 avx2 bmi2 erms xsaveopt arat Caches (sum of all): L1d: 288 KiB (9 instances) L1i: 288 KiB (9 instances) L2: 2.3 MiB (9 instances) L3: 108 MiB (9 instances) Vulnerabilities: Itlb multihit:KVM: Mitigation: VMX unsupported L1tf: Mitigation; PTE Inversion Mds: Vulnerable; SMT Host state unknown Meltdown: Vulnerable Mmio stale data: Vulnerable Spec store bypass:Vulnerable Spectre v1: Vulnerable: __user pointer sanitization and usercopy barriers only; no swapgs barriers Spectre v2: Vulnerable, STIBP: disabled Srbds:Unknown: Dependent on hypervisor status Tsx async abort: Not affected [edb@9428da9d2137 postgres]$ clang --version clang version 15.0.7 (Red Hat 15.0.7-2.el9) Target: s390x-ibm-linux-gnu Thread model: posix InstalledDir: /usr/bin [edb@9428da9d2137 postgres]$ rpm -qa | grep llvm *llvm*-libs-15.0.7-1.el9.s390x *llvm*-15.0.7-1.el9.s390x *llvm*-test-15.0.7-1.el9.s390x *llvm*-static-15.0.7-1.el9.s390x *llvm*-devel-15.0.7-1.el9.s390x Please let me know if any further information is required. > > > > *Test case:* > > CREATE TABLE rm32044_t1 > > ( > > pkey integer, > > val text > > ); > > CREATE TABLE rm32044_t2 > > ( > > pkey integer, > > label text, > > hidden boolean > > ); > > CREATE TABLE rm32044_t3 > > ( > > pkey integer, > > val integer > > ); > > CREATE TABLE rm32044_t4 > > ( > > pkey integer > > ); > > insert into rm32044_t1 values ( 1 , 'row1'); > > insert into rm32044_t1 values ( 2 , 'row2'); > > insert into rm32044_t2 values ( 1 , 'hidden', true); > > insert into rm32044_t2 values ( 2 , 'visible', false); > > insert into rm32044_t3 values (1 , 1); > > insert into rm32044_t3 values (2 , 1); > > > > postgres=# SELECT * FROM rm32044_t1 LEFT JOIN rm32044_t2 ON > rm32044_t1.pkey > > = rm32044_t2.pk
Re: Server crash on RHEL 9/s390x platform against PG16
Here is clang version:
[edb@9428da9d2137]$ clang --version
clang version 15.0.7 (Red Hat 15.0.7-2.el9)
Target: s390x-ibm-linux-gnu
Thread model: posix
InstalledDir: /usr/bin
Let me know if any further information is needed.
On Mon, Oct 9, 2023 at 8:21 AM Suraj Kharage
wrote:
> It looks like an issue with JIT. If I disable the JIT then the above query
> runs successfully.
>
> postgres=# set jit to off;
>
> SET
>
> postgres=# SELECT * FROM rm32044_t1 LEFT JOIN rm32044_t2 ON
> rm32044_t1.pkey = rm32044_t2.pkey, rm32044_t3 LEFT JOIN rm32044_t4 ON
> rm32044_t3.pkey = rm32044_t4.pkey order by rm32044_t1.pkey,label,hidden;
>
> pkey | val | pkey | label | hidden | pkey | val | pkey
>
> --+--+--+-++--+-+--
>
> 1 | row1 |1 | hidden | t |1 | 1 |
>
> 1 | row1 |1 | hidden | t |2 | 1 |
>
> 2 | row2 |2 | visible | f |1 | 1 |
>
> 2 | row2 |2 | visible | f |2 | 1 |
>
> (4 rows)
>
> Any idea on this?
>
> On Mon, Sep 18, 2023 at 11:20 AM Suraj Kharage <
> suraj.khar...@enterprisedb.com> wrote:
>
>> Few more details on this:
>>
>> (gdb) p val
>> $1 = 0
>> (gdb) p i
>> $2 = 3
>> (gdb) f 3
>> #3 0x01a1ef70 in ExecCopySlotMinimalTuple (slot=0x202e4f8) at
>> ../../../../src/include/executor/tuptable.h:472
>> 472 return slot->tts_ops->copy_minimal_tuple(slot);
>> (gdb) p *slot
>> $3 = {type = T_TupleTableSlot, tts_flags = 16, tts_nvalid = 8, tts_ops =
>> 0x1b6dcc8 , tts_tupleDescriptor = 0x202e0e8, tts_values =
>> 0x202e540, tts_isnull = 0x202e580, tts_mcxt = 0x1f54550, tts_tid =
>> {ip_blkid = {bi_hi = 65535,
>> bi_lo = 65535}, ip_posid = 0}, tts_tableOid = 0}
>> (gdb) p *slot->tts_tupleDescriptor
>> $2 = {natts = 8, tdtypeid = 2249, tdtypmod = -1, tdrefcount = -1, constr
>> = 0x0, attrs = 0x202cd28}
>>
>> (gdb) p slot.tts_values[3]
>> $4 = 0
>> (gdb) p slot.tts_values[2]
>> $5 = 1
>> (gdb) p slot.tts_values[1]
>> $6 = 34027556
>>
>>
>> As per the resultslot, it has 0 value for the third attribute (column
>> lable).
>> Im testing this on the docker container and facing some issues with gdb
>> hence could not able to debug it further.
>>
>> Here is a explain plan:
>>
>> postgres=# explain (verbose, costs off) SELECT * FROM rm32044_t1 LEFT
>> JOIN rm32044_t2 ON rm32044_t1.pkey = rm32044_t2.pkey, rm32044_t3 LEFT JOIN
>> rm32044_t4 ON rm32044_t3.pkey = rm32044_t4.pkey order by
>> rm32044_t1.pkey,label,hidden;
>>
>> QUERY PLAN
>>
>>
>> -
>> Incremental Sort
>>Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
>> rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
>> rm32044_t4.pkey
>>Sort Key: rm32044_t1.pkey, rm32044_t2.label, rm32044_t2.hidden
>>Presorted Key: rm32044_t1.pkey
>>-> Merge Left Join
>> Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
>> rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
>> rm32044_t4.pkey
>> Merge Cond: (rm32044_t1.pkey = rm32044_t2.pkey)
>> -> Sort
>>Output: rm32044_t3.pkey, rm32044_t3.val, rm32044_t4.pkey,
>> rm32044_t1.pkey, rm32044_t1.val
>>Sort Key: rm32044_t1.pkey
>>-> Nested Loop
>> Output: rm32044_t3.pkey, rm32044_t3.val,
>> rm32044_t4.pkey, rm32044_t1.pkey, rm32044_t1.val
>> -> Merge Left Join
>>Output: rm32044_t3.pkey, rm32044_t3.val,
>> rm32044_t4.pkey
>>Merge Cond: (rm32044_t3.pkey = rm32044_t4.pkey)
>>-> Sort
>> Output: rm32044_t3.pkey, rm32044_t3.val
>> Sort Key: rm32044_t3.pkey
>> -> Seq Scan on public.rm32044_t3
>>Output: rm32044_t3.pkey,
>> rm32044_t3.val
>>-> Sort
>> Output: rm32044_t4.pkey
>> Sort Key: rm32044_t4.pkey
>> -> Seq Scan on public.rm32044_t4
>>Output: rm32044_t4.pkey
>> -> Materialize
>
Re: Server crash on RHEL 9/s390x platform against PG16
It looks like an issue with JIT. If I disable the JIT then the above query
runs successfully.
postgres=# set jit to off;
SET
postgres=# SELECT * FROM rm32044_t1 LEFT JOIN rm32044_t2 ON rm32044_t1.pkey
= rm32044_t2.pkey, rm32044_t3 LEFT JOIN rm32044_t4 ON rm32044_t3.pkey =
rm32044_t4.pkey order by rm32044_t1.pkey,label,hidden;
pkey | val | pkey | label | hidden | pkey | val | pkey
--+--+--+-++--+-+--
1 | row1 |1 | hidden | t |1 | 1 |
1 | row1 |1 | hidden | t |2 | 1 |
2 | row2 |2 | visible | f |1 | 1 |
2 | row2 |2 | visible | f |2 | 1 |
(4 rows)
Any idea on this?
On Mon, Sep 18, 2023 at 11:20 AM Suraj Kharage <
suraj.khar...@enterprisedb.com> wrote:
> Few more details on this:
>
> (gdb) p val
> $1 = 0
> (gdb) p i
> $2 = 3
> (gdb) f 3
> #3 0x01a1ef70 in ExecCopySlotMinimalTuple (slot=0x202e4f8) at
> ../../../../src/include/executor/tuptable.h:472
> 472 return slot->tts_ops->copy_minimal_tuple(slot);
> (gdb) p *slot
> $3 = {type = T_TupleTableSlot, tts_flags = 16, tts_nvalid = 8, tts_ops =
> 0x1b6dcc8 , tts_tupleDescriptor = 0x202e0e8, tts_values =
> 0x202e540, tts_isnull = 0x202e580, tts_mcxt = 0x1f54550, tts_tid =
> {ip_blkid = {bi_hi = 65535,
> bi_lo = 65535}, ip_posid = 0}, tts_tableOid = 0}
> (gdb) p *slot->tts_tupleDescriptor
> $2 = {natts = 8, tdtypeid = 2249, tdtypmod = -1, tdrefcount = -1, constr =
> 0x0, attrs = 0x202cd28}
>
> (gdb) p slot.tts_values[3]
> $4 = 0
> (gdb) p slot.tts_values[2]
> $5 = 1
> (gdb) p slot.tts_values[1]
> $6 = 34027556
>
>
> As per the resultslot, it has 0 value for the third attribute (column
> lable).
> Im testing this on the docker container and facing some issues with gdb
> hence could not able to debug it further.
>
> Here is a explain plan:
>
> postgres=# explain (verbose, costs off) SELECT * FROM rm32044_t1 LEFT JOIN
> rm32044_t2 ON rm32044_t1.pkey = rm32044_t2.pkey, rm32044_t3 LEFT JOIN
> rm32044_t4 ON rm32044_t3.pkey = rm32044_t4.pkey order by
> rm32044_t1.pkey,label,hidden;
>
> QUERY PLAN
>
>
> -
> Incremental Sort
>Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
> rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
> rm32044_t4.pkey
>Sort Key: rm32044_t1.pkey, rm32044_t2.label, rm32044_t2.hidden
>Presorted Key: rm32044_t1.pkey
>-> Merge Left Join
> Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
> rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
> rm32044_t4.pkey
> Merge Cond: (rm32044_t1.pkey = rm32044_t2.pkey)
> -> Sort
>Output: rm32044_t3.pkey, rm32044_t3.val, rm32044_t4.pkey,
> rm32044_t1.pkey, rm32044_t1.val
>Sort Key: rm32044_t1.pkey
>-> Nested Loop
> Output: rm32044_t3.pkey, rm32044_t3.val,
> rm32044_t4.pkey, rm32044_t1.pkey, rm32044_t1.val
> -> Merge Left Join
>Output: rm32044_t3.pkey, rm32044_t3.val,
> rm32044_t4.pkey
>Merge Cond: (rm32044_t3.pkey = rm32044_t4.pkey)
>-> Sort
> Output: rm32044_t3.pkey, rm32044_t3.val
> Sort Key: rm32044_t3.pkey
> -> Seq Scan on public.rm32044_t3
>Output: rm32044_t3.pkey,
> rm32044_t3.val
>-> Sort
> Output: rm32044_t4.pkey
> Sort Key: rm32044_t4.pkey
> -> Seq Scan on public.rm32044_t4
>Output: rm32044_t4.pkey
> -> Materialize
>Output: rm32044_t1.pkey, rm32044_t1.val
>-> Seq Scan on public.rm32044_t1
> Output: rm32044_t1.pkey, rm32044_t1.val
> -> Sort
>Output: rm32044_t2.pkey, rm32044_t2.label, rm32044_t2.hidden
>Sort Key: rm32044_t2.pkey
> -> Seq Scan on public.rm32044_t2
> Output: rm32044_t2.pkey, rm32044_t2.label,
> rm32044_t2.hidden
> (34 rows)
>
>
> It seems like while building the innerslot for merge join, the value for
> attnum 1 is not getting fetched correctly.
>
> On Tue, Sep 12, 2023 at 3:27 PM Suraj Kharage <
> sur
Re: pg_upgrade --check fails to warn about abstime
On Fri, Sep 22, 2023 at 4:44 PM Alvaro Herrera wrote: > On 2023-Sep-21, Tom Lane wrote: > > > Bruce Momjian writes: > > > > Wow, I never added code to pg_upgrade to check for that, and no one > > > complained either. > > > > Yeah, so most people had indeed listened to warnings and moved away > > from those datatypes. I'm inclined to think that adding code for this > > at this point is a bit of a waste of time. > > The migrations from versions prior to 12 have not stopped yet, and I did > receive a complaint about it. Because the change is so simple, I'm > inclined to patch it anyway, late though it is. > > I decided to follow Tristan's advice to add the version number as a > parameter to the new function; this way, the knowledge of where was what > dropped is all in the callsite and none in the function. It > looked a bit schizoid otherwise. > yeah, looks good to me. > > -- > Álvaro Herrera 48°01'N 7°57'E — > https://www.EnterpriseDB.com/ > "Postgres is bloatware by design: it was built to house > PhD theses." (Joey Hellerstein, SIGMOD annual conference 2002) > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: pg_upgrade --check fails to warn about abstime
Thanks, Alvaro, for working on this. The patch looks good to me. + * similar to the above, but for types that were removed in 12. Comment can start with a capital letter. Also, We need to backport the same, right? On Wed, Sep 20, 2023 at 10:24 PM Alvaro Herrera wrote: > I got a complaint that pg_upgrade --check fails to raise red flags when > the source database contains type abstime when upgrading from pg11. The > type (along with reltime and tinterval) was removed by pg12. > > > In passing, while testing this, I noticed that the translation > infrastructure in pg_upgrade/util.c is broken: we do have the messages > in the translation catalog, but the translations for the messages from > prep_status are never displayed. So I made the quick hack of adding _() > around the fmt, and this was the result: > > Verificando Consistencia en Vivo en el Servidor Antiguo > --- > Verificando las versiones de los clústerséxito > Verificando que el usuario de base de datos es el usuario de > instalaciónéxito > Verificando los parámetros de conexión de bases de datoséxito > Verificando transacciones preparadas éxito > Verificando tipos compuestos definidos por el sistema en tablas de > usuarioéxito > Verificando tipos de datos reg* en datos de usuario éxito > Verificando contrib/isn con discordancia en mecanismo de paso de > bigintéxito > Checking for incompatible "aclitem" data type in user tables éxito > Checking for removed "abstime" data type in user tables éxito > Checking for removed "reltime" data type in user tables éxito > Checking for removed "tinterval" data type in user tables éxito > Verificando conversiones de codificación definidas por el usuarioéxito > Verificando operadores postfix definidos por el usuario éxito > Verificando funciones polimórficas incompatibles éxito > Verificando tablas WITH OIDS éxito > Verificando columnas de usuario del tipo «sql_identifier» éxito > Verificando la presencia de las bibliotecas requeridaséxito > Verificando que el usuario de base de datos es el usuario de > instalaciónéxito > Verificando transacciones preparadas éxito > Verificando los directorios de tablespaces para el nuevo clústeréxito > > Note how nicely they line up ... not. There is some code that claims to > do this correctly, but apparently it counts bytes, not characters, and > also it appears to be measuring the original rather than the > translation. > > I think we're trimming the strings in the wrong places. We need to > apply _() to the originals, not the trimmed ones. Anyway, clearly > nobody has looked at this very much. > > -- > Álvaro Herrera PostgreSQL Developer — > https://www.EnterpriseDB.com/ > "We’ve narrowed the problem down to the customer’s pants being in a > situation > of vigorous combustion" (Robert Haas, Postgres expert extraordinaire) > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: Server crash on RHEL 9/s390x platform against PG16
Few more details on this:
(gdb) p val
$1 = 0
(gdb) p i
$2 = 3
(gdb) f 3
#3 0x01a1ef70 in ExecCopySlotMinimalTuple (slot=0x202e4f8) at
../../../../src/include/executor/tuptable.h:472
472 return slot->tts_ops->copy_minimal_tuple(slot);
(gdb) p *slot
$3 = {type = T_TupleTableSlot, tts_flags = 16, tts_nvalid = 8, tts_ops =
0x1b6dcc8 , tts_tupleDescriptor = 0x202e0e8, tts_values =
0x202e540, tts_isnull = 0x202e580, tts_mcxt = 0x1f54550, tts_tid =
{ip_blkid = {bi_hi = 65535,
bi_lo = 65535}, ip_posid = 0}, tts_tableOid = 0}
(gdb) p *slot->tts_tupleDescriptor
$2 = {natts = 8, tdtypeid = 2249, tdtypmod = -1, tdrefcount = -1, constr =
0x0, attrs = 0x202cd28}
(gdb) p slot.tts_values[3]
$4 = 0
(gdb) p slot.tts_values[2]
$5 = 1
(gdb) p slot.tts_values[1]
$6 = 34027556
As per the resultslot, it has 0 value for the third attribute (column
lable).
Im testing this on the docker container and facing some issues with gdb
hence could not able to debug it further.
Here is a explain plan:
postgres=# explain (verbose, costs off) SELECT * FROM rm32044_t1 LEFT JOIN
rm32044_t2 ON rm32044_t1.pkey = rm32044_t2.pkey, rm32044_t3 LEFT JOIN
rm32044_t4 ON rm32044_t3.pkey = rm32044_t4.pkey order by
rm32044_t1.pkey,label,hidden;
QUERY PLAN
-
Incremental Sort
Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
rm32044_t4.pkey
Sort Key: rm32044_t1.pkey, rm32044_t2.label, rm32044_t2.hidden
Presorted Key: rm32044_t1.pkey
-> Merge Left Join
Output: rm32044_t1.pkey, rm32044_t1.val, rm32044_t2.pkey,
rm32044_t2.label, rm32044_t2.hidden, rm32044_t3.pkey, rm32044_t3.val,
rm32044_t4.pkey
Merge Cond: (rm32044_t1.pkey = rm32044_t2.pkey)
-> Sort
Output: rm32044_t3.pkey, rm32044_t3.val, rm32044_t4.pkey,
rm32044_t1.pkey, rm32044_t1.val
Sort Key: rm32044_t1.pkey
-> Nested Loop
Output: rm32044_t3.pkey, rm32044_t3.val,
rm32044_t4.pkey, rm32044_t1.pkey, rm32044_t1.val
-> Merge Left Join
Output: rm32044_t3.pkey, rm32044_t3.val,
rm32044_t4.pkey
Merge Cond: (rm32044_t3.pkey = rm32044_t4.pkey)
-> Sort
Output: rm32044_t3.pkey, rm32044_t3.val
Sort Key: rm32044_t3.pkey
-> Seq Scan on public.rm32044_t3
Output: rm32044_t3.pkey,
rm32044_t3.val
-> Sort
Output: rm32044_t4.pkey
Sort Key: rm32044_t4.pkey
-> Seq Scan on public.rm32044_t4
Output: rm32044_t4.pkey
-> Materialize
Output: rm32044_t1.pkey, rm32044_t1.val
-> Seq Scan on public.rm32044_t1
Output: rm32044_t1.pkey, rm32044_t1.val
-> Sort
Output: rm32044_t2.pkey, rm32044_t2.label, rm32044_t2.hidden
Sort Key: rm32044_t2.pkey
-> Seq Scan on public.rm32044_t2
Output: rm32044_t2.pkey, rm32044_t2.label,
rm32044_t2.hidden
(34 rows)
It seems like while building the innerslot for merge join, the value for
attnum 1 is not getting fetched correctly.
On Tue, Sep 12, 2023 at 3:27 PM Suraj Kharage <
suraj.khar...@enterprisedb.com> wrote:
> Hi,
>
> Found server crash on RHEL 9/s390x platform with below test case -
>
> *Machine details:*
>
>
>
>
>
>
>
> *[edb@9428da9d2137 postgres]$ cat /etc/redhat-release AlmaLinux release
> 9.2 (Turquoise Kodkod)[edb@9428da9d2137 postgres]$ lscpuArchitecture:
> s390x CPU op-mode(s): 32-bit, 64-bit Address sizes:39
> bits physical, 48 bits virtual Byte Order: Big Endian*
> *Configure command:*
> ./configure --prefix=/home/edb/postgres/ --with-lz4 --with-zstd
> --with-llvm --with-perl --with-python --with-tcl --with-openssl
> --enable-nls --with-libxml --with-libxslt --with-systemd --with-libcurl
> --without-icu --enable-debug --enable-cassert --with-pgport=5414
>
>
> *Test case:*
> CREATE TABLE rm32044_t1
> (
> pkey integer,
> val text
> );
> CREATE TABLE rm32044_t2
> (
> pkey integer,
> label text,
> hidden boolean
> );
> CREATE TABLE rm32044_t3
> (
> pkey integer,
> val integer
> );
> CREATE TABLE rm32044_t4
> (
> pkey integer
> );
> insert into rm32044_t1 values ( 1 , 'row1');
> insert
Server crash on RHEL 9/s390x platform against PG16
& Regards, Suraj kharage, edbpostgres.com
Re: [Regression] Incorrect filename in test case comment
Thanks Daniel and Michael. On Wed, Sep 6, 2023 at 1:52 PM Daniel Gustafsson wrote: > > On 6 Sep 2023, at 10:19, Michael Paquier wrote: > > > > On Wed, Sep 06, 2023 at 10:48:32AM +0530, Suraj Kharage wrote: > >> While browsing the test cases, found that the incorrect filename was > there > >> in the test case comment. > >> The below commit added the custom hash opclass in insert.sql, > > > > --- part_part_test_int4_ops and part_test_text_ops in insert.sql. > > +-- part_part_test_int4_ops and part_test_text_ops in test_setup.sql. > > > > Good catch, but part_part_test_int4_ops should be renamed to > > part_test_int4_ops, removing the first "part_", no? > > Ah, seems we came to same conclusion when looking simultaneously, I just > pushed > the fix with the typo fix. > > -- > Daniel Gustafsson > > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
[Regression] Incorrect filename in test case comment
Hi, While browsing the test cases, found that the incorrect filename was there in the test case comment. The below commit added the custom hash opclass in insert.sql, -- *commit fafec4cce814b9b15991b62520dc5e5e84655a8aAuthor: Alvaro Herrera >Date: Fri Apr 13 12:27:22 2018 -0300Use custom hash opclass for hash partition pruning* -- and later below commit moved those to test_setup.sql -- *commit cc50080a828dd4791b43539f5a0f976e535d147cAuthor: Tom Lane >Date: Tue Feb 8 15:30:38 2022 -0500* *Rearrange core regression tests to reduce cross-script dependencies. * -- but we haven't changed the filename in other test cases. Did the same in the attached patch. -- -- Thanks & Regards, Suraj kharage, edbpostgres.com Fix_file_name_in_test_case.patch Description: Binary data
Re: postgres_fdw: wrong results with self join + enable_nestloop off
+1 for fixing this in the backend code rather than FDW code. Thanks, Richard, for working on this. The patch looks good to me at a glance. On Tue, Apr 25, 2023 at 3:36 PM Richard Guo wrote: > > On Fri, Apr 14, 2023 at 8:51 PM Etsuro Fujita > wrote: > >> I think that the root cause for this issue would be in the >> create_scan_plan handling of pseudoconstant quals when creating a >> foreign-join (or custom-join) plan. > > > Yes exactly. In create_scan_plan, we are supposed to extract all the > pseudoconstant clauses and use them as one-time quals in a gating Result > node. Currently we check against rel->baserestrictinfo and ppi_clauses > for the pseudoconstant clauses. But for scans of foreign joins, we do > not have any restriction clauses in these places and thus the gating > Result node as well as the pseudoconstant clauses would just be lost. > > I looked at Nishant's patch. IIUC it treats the pseudoconstant clauses > as local conditions. While it can fix the wrong results issue, I think > maybe it's better to still treat the pseudoconstant clauses as one-time > quals in a gating node. So I wonder if we can store the restriction > clauses for foreign joins in ForeignPath, just as what we do for normal > JoinPath, and then check against them for pseudoconstant clauses in > create_scan_plan, something like attached. > > BTW, while going through the codes I noticed one place in > add_foreign_final_paths that uses NULL for List *. I changed it to NIL. > > Thanks > Richard > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Remove extra spaces
Hi, While browsing the code, noticed the extra spaces after the function name. Removed the same in the attached patch. -- -- Thanks & Regards, Suraj kharage, edbpostgres.com remove_space_from_numeric_function.patch Description: Binary data
Re: [PATCH] improve the pg_upgrade error message
Thanks Jeevan for working on this.
Overall patch looks good to me.
+ pg_fatal("All non-template0 databases must allow connections, i.e.
their\n"
+ "pg_database.datallowconn must be true. Your installation contains\n"
+ "non-template0 databases with their pg_database.datallowconn set to\n"
+ "false. Consider allowing connection for all non-template0 databases\n"
+ "using:\n"
+ "UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname
NOT LIKE 'template0';\n"
+ "A list of databases with the problem is given in the file:\n"
+ "%s\n\n", output_path);
Instead of giving suggestion about updating the pg_database catalog, can we
give "ALTER DATABASE ALLOW_CONNECTIONS true;" command?
Also, it would be good if we give 2 spaces after full stop in an error
message.
On Tue, Jul 13, 2021 at 6:57 PM Jeevan Ladhe
wrote:
>
> The admin might fix DB123, restart their upgrade procedure, spend 5 or 15
>> minutes with that, only to have it then fail on DB1234.
>>
>
> Agree with this observation.
>
> Here is a patch that writes the list of all the databases other than
> template0
> that are having their pg_database.datallowconn to false in a file. Similar
> approach is seen in other functions like check_for_data_types_usage(),
> check_for_data_types_usage() etc. Thanks Suraj Kharage for the offline
> suggestion.
>
> PFA patch.
>
> For experiment, here is how it turns out after the fix.
>
> postgres=# update pg_database set datallowconn='false' where datname in
> ('mydb', 'mydb1', 'mydb2');
> UPDATE 3
>
> $ pg_upgrade -d /tmp/v96/data -D /tmp/v13/data -b $HOME/v96/install/bin -B
> $HOME/v13/install/bin
> Performing Consistency Checks
> -
> Checking cluster versions ok
> Checking database user is the install user ok
> Checking database connection settings fatal
>
> All non-template0 databases must allow connections, i.e. their
> pg_database.datallowconn must be true. Your installation contains
> non-template0 databases with their pg_database.datallowconn set to
> false. Consider allowing connection for all non-template0 databases
> using:
> UPDATE pg_catalog.pg_database SET datallowconn='true' WHERE datname
> NOT LIKE 'template0';
> A list of databases with the problem is given in the file:
> databases_with_datallowconn_false.txt
>
> Failure, exiting
>
> $ cat databases_with_datallowconn_false.txt
> mydb
> mydb1
> mydb2
>
>
> Regards,
> Jeevan Ladhe
>
--
--
Thanks & Regards,
Suraj kharage,
edbpostgres.com
Re: [PATCH] improve the pg_upgrade error message
+1 for the change. Patch looks good to me. On Mon, Jul 12, 2021 at 4:59 PM Jeevan Ladhe wrote: > While looking into one of the pg_upgrade issue, I found it > > challenging to find out the database that has the datallowconn set to > > 'false' that was throwing following error: > > > *"All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true"* > > > edb=# create database mydb; > > CREATE DATABASE > > edb=# update pg_database set datallowconn='false' where datname like > 'mydb'; > > UPDATE 1 > > > Now, when I try to upgrade the server, without the patch we get above > > error, which leaves no clue behind about which database has datallowconn > > set to 'false'. It can be argued that we can query the pg_database > > catalog and find that out easily, but at times it is challenging to get > > that from the customer environment. But, anyways I feel we have scope to > > improve the error message here per the attached patch. > > > With attached patch, now I get following error: > > *"All non-template0 databases must allow connections, i.e. their > pg_database.datallowconn must be true; database "mydb" has datallowconn set > to false."* > > > > Regards, > > Jeevan Ladhe > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: Query about time zone patterns in to_char
+1 for the change. I quickly reviewed the patch and overall it looks good to me. Few cosmetic suggestions: 1: +RESET timezone; + + CREATE TABLE TIMESTAMPTZ_TST (a int , b timestamptz); Extra line. 2: +SET timezone = '00:00'; +SELECT to_char(now(), 'of') as "Of", to_char(now(), 'tzh:tzm') as "tzh:tzm"; O should be small in alias just for consistency. I am not sure whether we should backport this or not but I don't see any issues with back-patching. On Sun, May 16, 2021 at 9:43 PM Nitin Jadhav wrote: > > AFAICS, table 9.26 specifically shows which case-variants are supported. > > If there are some others that happen to work, we probably shouldn't > > remove them for fear of breaking poorly-written apps ... but that does > > not imply that we need to support every case-variant. > > Thanks for the explanation. I also feel that we may not support every > case-variant. But the other reason which triggered me to think in the > other way is, as mentioned in commit [1] where this feature was added, > says that these format patterns are compatible with Oracle. Whereas > Oracle supports both upper case and lower case patterns. I just wanted > to get it confirmed with this point before concluding. > > [1] - > commit 11b623dd0a2c385719ebbbdd42dd4ec395dcdc9d > Author: Andrew Dunstan > Date: Tue Jan 9 14:25:05 2018 -0500 > > Implement TZH and TZM timestamp format patterns > > These are compatible with Oracle and required for the datetime template > language for jsonpath in an upcoming patch. > > Nikita Glukhov and Andrew Dunstan, reviewed by Pavel Stehule. > > Thanks & Regards, > Nitin Jadhav > > On Sun, May 16, 2021 at 8:40 PM Tom Lane wrote: > > > > Nitin Jadhav writes: > > > While understanding the behaviour of the to_char() function as > > > explained in [1], I observed that some patterns related to time zones > > > do not display values if we mention in lower case. As shown in the > > > sample output [2], time zone related patterns TZH, TZM and OF outputs > > > proper values when specified in upper case but does not work if we > > > mention in lower case. But other patterns like TZ, HH, etc works fine > > > with upper case as well as lower case. > > > > > I would like to know whether the current behaviour of TZH, TZM and OF > > > is done intentionally and is as expected. > > > > AFAICS, table 9.26 specifically shows which case-variants are supported. > > If there are some others that happen to work, we probably shouldn't > > remove them for fear of breaking poorly-written apps ... but that does > > not imply that we need to support every case-variant. > > > > regards, tom lane > > > -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: CREATE SEQUENCE with RESTART option
On Thu, Apr 8, 2021 at 2:03 PM Bharath Rupireddy < bharath.rupireddyforpostg...@gmail.com> wrote: > > > > > The RESTART clause in the CREATE SEQUENCE doesn't make sense > > to me, it should be restricted, IMO. > +1 > > Thanks! Attaching a patch that throws an error if the RESTART option > is specified with CREATE SEQUENCE. Please have a look and let me know > if the error message wording is fine or not. Is it better to include > the reason as to why we disallow something like "Because it may > override the START option." in err_detail along with the error > message? > Patch looks good to me. Current error message looks ok to me. Do we need to add double quotes for RESTART word in the error message since it is an option? -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
extra semicolon in postgres_fdw test cases
Hi, Noticed that an extra semicolon in a couple of test cases related to postgres_fdw. Removed that in the attached patch. It can be backported till v11 where we added those test cases. -- -- Thanks & Regards, Suraj kharage, edbpostgres.com remove_extra_semicolon_postgres_fdw.patch Description: Binary data
Re: refactoring basebackup.c
On Tue, Jun 30, 2020 at 10:45 AM Dipesh Pandit wrote: > Hi, > > I have repeated the experiment with 8K block size and found that the > results are not varying much after applying the patch. > Please find the details below. > > > Later I connected with Suraj to validate the experiment details and found > that the setup and steps followed are exactly the same in this > experiment when compared with the previous experiment. > > Thanks Dipesh. It looks like the results are not varying much with your run as you followed the same steps. One of my run with 8kb which took more time than others might be because of noise at that time. -- -- Thanks & Regards, Suraj kharage, edbpostgres.com
Re: refactoring basebackup.c
Hi, On Wed, May 13, 2020 at 7:49 PM Robert Haas wrote: > > So the patch came out slightly faster at 8kB and slightly slower in the > other tests. That's kinda strange. I wonder if it's just noise. How much do > the results vary run to run? > It is not varying much except for 8kB run. Please see below details for both runs of each scenario. 8kb 32kb (default value) 128kB 1024kB WIthout refactor patch 1st run real 10m50.924s user 1m29.774s sys 9m13.058s real 8m36.245s user 1m8.471s sys 7m21.520s real 7m8.690s user 0m54.840s sys 6m1.725s real 18m16.898s user 1m39.105s sys 9m42.803s 2nd run real 10m22.718s user 1m23.629s sys 8m51.410s real 8m44.455s user 1m7.896s sys 7m28.909s real 6m54.299s user 0m55.690s sys 5m46.502s real 18m3.511s user 1m38.197s sys 9m36.517s WIth refactor patch 1st run real 10m11.350s user 1m25.038s sys 8m39.226s real 8m56.226s user 1m9.774s sys 7m41.032s real 7m26.678s user 0m54.833s sys 6m20.057s real 19m5.218s user 1m44.122s sys 10m17.623s 2nd run real 11m30.500s user 1m45.221s sys 9m37.815s real 9m4.103s user 1m6.893s sys 7m49.393s real 7m26.713s user 0m54.868s sys 6m19.652s real 18m17.230s user 1m42.749s sys 9m53.704s -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: refactoring basebackup.c
Hi, Did some performance testing by varying TAR_SEND_SIZE with Robert's refactor patch and without the patch to check the impact. Below are the details: *Backup type*: local backup using pg_basebackup *Data size*: Around 200GB (200 tables - each table around 1.05 GB) *different TAR_SEND_SIZE values*: 8kb, 32kb (default value), 128kB, 1MB ( 1024kB) *Server details:* RAM: 500 GB CPU details: Architecture: x86_64 CPU op-mode(s): 32-bit, 64-bit Byte Order: Little Endian CPU(s): 128 Filesystem: ext4 8kb 32kb (default value) 128kB 1024kB Without refactor patch real 10m22.718s user 1m23.629s sys 8m51.410s real 8m36.245s user 1m8.471s sys 7m21.520s real 6m54.299s user 0m55.690s sys 5m46.502s real 18m3.511s user 1m38.197s sys 9m36.517s With refactor patch (Robert's patch) real 10m11.350s user 1m25.038s sys 8m39.226s real 8m56.226s user 1m9.774s sys 7m41.032s real 7m26.678s user 0m54.833s sys 6m20.057s real 18m17.230s user 1m42.749s sys 9m53.704s The above numbers are taken from the minimum of two runs of each scenario. I can see, when we have TAR_SEND_SIZE as 32kb or 128kb, it is giving us a good performance whereas, for 1Mb it is taking 2.5x more time. Please let me know your thoughts/suggestions on the same. -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: backup manifests
One more suggestion, recent commit (1933ae62) has added the PostgreSQL home
page to --help output.
e.g:
*PostgreSQL home page: <https://www.postgresql.org/
<https://www.postgresql.org/>>*
We might need to consider this change for pg_validatebackup binary.
On Mon, Mar 16, 2020 at 10:37 AM Suraj Kharage <
suraj.khar...@enterprisedb.com> wrote:
> Thank you, Robert.
>
> Getting below warning while compiling the
> v11-0003-pg_validatebackup-Validate-a-backup-against-the-.patch.
>
>
>
> *pg_validatebackup.c: In function
> ‘report_manifest_error’:pg_validatebackup.c:356:2: warning: function might
> be possible candidate for ‘gnu_printf’ format attribute
> [-Wsuggest-attribute=format] pg_log_generic_v(PG_LOG_FATAL, fmt, ap);*
>
>
> To resolve this, can we use "pg_attribute_printf(2, 3)" in function
> declaration something like below?
> e.g:
>
> diff --git a/src/bin/pg_validatebackup/parse_manifest.h
> b/src/bin/pg_validatebackup/parse_manifest.h
> index b0b18a5..25d140f 100644
> --- a/src/bin/pg_validatebackup/parse_manifest.h
> +++ b/src/bin/pg_validatebackup/parse_manifest.h
> @@ -25,7 +25,7 @@ typedef void
> (*json_manifest_perfile_callback)(JsonManifestParseContext *,
> size_t
> size, pg_checksum_type checksum_type,
> int
> checksum_length, uint8 *checksum_payload);
> typedef void (*json_manifest_error_callback)(JsonManifestParseContext *,
> -char
> *fmt, ...);
> +char
> *fmt,...) pg_attribute_printf(2, 3);
>
> struct JsonManifestParseContext
> {
> diff --git a/src/bin/pg_validatebackup/pg_validatebackup.c
> b/src/bin/pg_validatebackup/pg_validatebackup.c
> index 0e7299b..6ccbe59 100644
> --- a/src/bin/pg_validatebackup/pg_validatebackup.c
> +++ b/src/bin/pg_validatebackup/pg_validatebackup.c
> @@ -95,7 +95,7 @@ static void
> record_manifest_details_for_file(JsonManifestParseContext *context,
>
>int checksum_length,
>
>uint8 *checksum_payload);
> static void report_manifest_error(JsonManifestParseContext *context,
> - char
> *fmt, ...);
> + char
> *fmt,...) pg_attribute_printf(2, 3);
>
> static void validate_backup_directory(validator_context *context,
>
> char *relpath, char *fullpath);
>
>
> Typos:
>
> 0004 patch
> unexpctedly => unexpectedly
>
> 0005 patch
> bacup => backup
>
> On Sat, Mar 14, 2020 at 2:04 AM Robert Haas wrote:
>
>> On Thu, Mar 12, 2020 at 10:47 AM tushar
>> wrote:
>> > On 3/9/20 10:46 PM, Robert Haas wrote:
>> > > Seems like expected behavior to me. We could consider providing a more
>> > > descriptive error message, but there's now way for it to work.
>> >
>> > Right , Error message need to be more user friendly .
>>
>> OK. Done in the attached version, which also includes a few other changes:
>>
>> - I expanded the regression tests. They now cover every line of code
>> in parse_manifest.c except for a few that I believe to be unreachable
>> (though I might be mistaken). Coverage for pg_validatebackup.c is also
>> improved, but it's not 100%; there are some cases that I don't know
>> how to hit outside of a kernel malfunction, and others that I only
>> know how to hit on non-Windows systems. For instance, it's easy to use
>> perl to make a file inaccessible on Linux with chmod(0, $filename),
>> but I gather that doesn't work on Windows. I'm going to spend a bit
>> more time looking at this, but I think it's already reasonably good.
>>
>> - I fixed a couple of very minor bugs which I discovered by writing those
>> tests.
>>
>> - I added documentation, in part based on a draft Mark Dilger shared
>> with me off-list.
>>
>> I don't think this is committable just yet, but I think it's getting
>> fairly close, so if anyone has major objections please speak up soon.
>>
>> --
>> Robert Haas
>> EnterpriseDB: http://www.enterprisedb.com
>> The Enterprise PostgreSQL Company
>>
>
>
> --
> --
>
> Thanks & Regards,
> Suraj kharage,
> EnterpriseDB Corporation,
> The Postgres Database Company.
>
--
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
Re: backup manifests
Thank you, Robert.
Getting below warning while compiling the
v11-0003-pg_validatebackup-Validate-a-backup-against-the-.patch.
*pg_validatebackup.c: In function
‘report_manifest_error’:pg_validatebackup.c:356:2: warning: function might
be possible candidate for ‘gnu_printf’ format attribute
[-Wsuggest-attribute=format] pg_log_generic_v(PG_LOG_FATAL, fmt, ap);*
To resolve this, can we use "pg_attribute_printf(2, 3)" in function
declaration something like below?
e.g:
diff --git a/src/bin/pg_validatebackup/parse_manifest.h
b/src/bin/pg_validatebackup/parse_manifest.h
index b0b18a5..25d140f 100644
--- a/src/bin/pg_validatebackup/parse_manifest.h
+++ b/src/bin/pg_validatebackup/parse_manifest.h
@@ -25,7 +25,7 @@ typedef void
(*json_manifest_perfile_callback)(JsonManifestParseContext *,
size_t
size, pg_checksum_type checksum_type,
int
checksum_length, uint8 *checksum_payload);
typedef void (*json_manifest_error_callback)(JsonManifestParseContext *,
-char *fmt,
...);
+char
*fmt,...) pg_attribute_printf(2, 3);
struct JsonManifestParseContext
{
diff --git a/src/bin/pg_validatebackup/pg_validatebackup.c
b/src/bin/pg_validatebackup/pg_validatebackup.c
index 0e7299b..6ccbe59 100644
--- a/src/bin/pg_validatebackup/pg_validatebackup.c
+++ b/src/bin/pg_validatebackup/pg_validatebackup.c
@@ -95,7 +95,7 @@ static void
record_manifest_details_for_file(JsonManifestParseContext *context,
int checksum_length,
uint8 *checksum_payload);
static void report_manifest_error(JsonManifestParseContext *context,
- char
*fmt, ...);
+ char
*fmt,...) pg_attribute_printf(2, 3);
static void validate_backup_directory(validator_context *context,
char *relpath, char *fullpath);
Typos:
0004 patch
unexpctedly => unexpectedly
0005 patch
bacup => backup
On Sat, Mar 14, 2020 at 2:04 AM Robert Haas wrote:
> On Thu, Mar 12, 2020 at 10:47 AM tushar
> wrote:
> > On 3/9/20 10:46 PM, Robert Haas wrote:
> > > Seems like expected behavior to me. We could consider providing a more
> > > descriptive error message, but there's now way for it to work.
> >
> > Right , Error message need to be more user friendly .
>
> OK. Done in the attached version, which also includes a few other changes:
>
> - I expanded the regression tests. They now cover every line of code
> in parse_manifest.c except for a few that I believe to be unreachable
> (though I might be mistaken). Coverage for pg_validatebackup.c is also
> improved, but it's not 100%; there are some cases that I don't know
> how to hit outside of a kernel malfunction, and others that I only
> know how to hit on non-Windows systems. For instance, it's easy to use
> perl to make a file inaccessible on Linux with chmod(0, $filename),
> but I gather that doesn't work on Windows. I'm going to spend a bit
> more time looking at this, but I think it's already reasonably good.
>
> - I fixed a couple of very minor bugs which I discovered by writing those
> tests.
>
> - I added documentation, in part based on a draft Mark Dilger shared
> with me off-list.
>
> I don't think this is committable just yet, but I think it's getting
> fairly close, so if anyone has major objections please speak up soon.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>
--
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
Re: backup manifests
Thanks, Robert. 1: Getting below error while compiling 0002 patch. edb@localhost:postgres$ mi > mi.log basebackup.c: In function ‘AddFileToManifest’: basebackup.c:1052:6: error: ‘pathname’ undeclared (first use in this function) pathname); ^ basebackup.c:1052:6: note: each undeclared identifier is reported only once for each function it appears in make[3]: *** [basebackup.o] Error 1 make[2]: *** [replication-recursive] Error 2 make[1]: *** [install-backend-recurse] Error 2 make: *** [install-src-recurse] Error 2 I can see you have renamed the filename argument of AddFileToManifest() to pathname, but those changes are part of 0003 (validator patch). I think the changes related to src/backend/replication/basebackup.c should not be there in the validator patch (0003). We can move these changes to backup manifest patch, either in 0002 or 0004 for better readability of patch set. 2: #define KW_MANIFEST_VERSION "PostgreSQL-Backup-Manifest-Version" #define KW_MANIFEST_FILE "File" #define KW_MANIFEST_CHECKSUM "Manifest-Checksum" #define KWL_MANIFEST_VERSION (sizeof(KW_MANIFEST_VERSION)-1) #define KWL_MANIFEST_FILE (sizeof(KW_MANIFEST_FILE)-1) #define KWL_MANIFEST_CHECKSUM (sizeof(KW_MANIFEST_CHECKSUM)-1) #define FIELDS_PER_FILE_LINE 4 Few macros defined in 0003 patch not used anywhere in 0005 patch. Either we can replace these with hard-coded values or remove them. On Thu, Mar 5, 2020 at 10:25 PM Robert Haas wrote: > On Thu, Mar 5, 2020 at 7:05 AM tushar > wrote: > > There is one small observation if we use slash (/) with option -i then > not getting the desired result > > Here's an updated patch set responding to many of the comments > received thus far. Since there are quite a few emails, let me > consolidate my comments and responses here. > > Report: Segmentation fault if -m is used to point to a valid manifest, > but actual backup directory is nonexistent. > Response: Fixed; thanks for the report. > > Report: pg_validatebackup doesn't complain about problems within the > pg_wal directory. > Response: That's out of scope. The WAL files are fetched separately > and are therefore not part of the manifest. > > Report: Inaccessible file in data directory being validated leads to a > double free. > Response: Fixed; thanks for the report. > > Report: Patch 0005 doesn't validate the manifest checksum. > Response: I know. I mentioned that when posting the previous patch > set. Fixed in this version, though. > > Report: Removing an empty directory doesn't make backup validation > fail, even though it might cause problems for the server. > Response: That's a little unfortunate, but I'm not sure it's really > worth complicating the patch to deal with it. It's something of a > corner case. > > Report: Negative file sizes in the backup manifest are interpreted as > large integers. > Response: That's also a little unfortunate, but I doubt it's worth > adding code to catch it, since any such manifest is corrupt. Also, > it's not like we're ignoring it; the error just isn't ideal. > > Report: If I take the backup label from backup #1 and stick it into > otherwise-identical backup #2, validation succeeds but the server > won't start. > Response: That's because we can't validate the pg_wal directory. As > noted above, that's out of scope. > > Report: Using --ignore with a slash-terminated pathname doesn't work > as expected. > Response: Fixed, thanks for the report. > > Off-List Report: You forgot a PG_BINARY flag. > Response: Fixed. I thought I'd done this before but there were two > places and I'd only fixed one of them. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: backup manifests
On Wed, Mar 4, 2020 at 7:21 PM tushar wrote: > Hi, > > There is a scenario in which i add something inside the pg_tablespace > directory , i am getting an error like- > > pg_validatebackup: * manifest_checksum = > 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04 > pg_validatebackup: error: "pg_tblspc/16385/*PG_13_202002271*/test" is > present on disk but not in the manifest > > but if i remove 'PG_13_202002271 ' directory then there is no error > > [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data > pg_validatebackup: * manifest_checksum = > 77ddacb4e7e02e2b880792a19a3adf09266dd88553dd15cfd0c22caee7d9cc04 > pg_validatebackup: backup successfully verified > > This seems expected considering current design as we don't log the directory entries in backup_manifest. In your case, you have tablespace with no objects (empty tablespace) then backup_manifest does not have any entry for this hence when you remove this tablespace directory, validator could not detect it. We can either document it or add the entry for directories in the manifest. Robert may have a better idea on this. -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: backup manifests
On Wed, Mar 4, 2020 at 3:51 PM tushar wrote: > Another scenario, in which if we modify Manifest-Checksum" value from > backup_manifest file , we are not getting an error > > [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/ > pg_validatebackup: * manifest_checksum = > 28d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d > pg_validatebackup: backup successfully verified > > open backup_manifest file and replace > > "Manifest-Checksum": > "8d082921650d0ae881de8ceb122c8d2af5f449f51ecfb446827f7f49f91f65d"} > with > "Manifest-Checksum": "Hello World"} > > rerun the pg_validatebackup > > [centos@tushar-ldap-docker bin]$ ./pg_validatebackup data/ > pg_validatebackup: * manifest_checksum = Hello World > pg_validatebackup: backup successfully verified > > regards, > Yeah, This handling is missing in the provided WIP patch. I believe Robert will consider this fixing in upcoming version of validator patch. -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: allow frontend use of the backend's core hashing functions
Hi, I have spent some time reviewing the patches and overall it looks good to me. However, I have few cosmetic review comments for 0003 patch as below; 1: +++ b/src/backend/utils/hash/hashfn.c @@ -16,15 +16,14 @@ * It is expected that every bit of a hash function's 32-bit result is * as random as every other; failure to ensure this is likely to lead * to poor performance of hash tables. In most cases a hash - * function should use hash_any() or its variant hash_uint32(). + * function should use hash_bytes() or its variant hash_bytes_uint32(), + * or the wrappers hash_any() and *hash_any_uint32* defined in hashfn.h. Here, indicated function name should be *hash_uint32*. 2: I can see renamed functions are declared twice in hashutils.c. I think duplicate declarations after #endif can be removed, +extern uint32 hash_bytes(const unsigned char *k, int keylen); +extern uint64 hash_bytes_extended(const unsigned char *k, + int keylen, uint64 seed); +extern uint32 hash_bytes_uint32(uint32 k); +extern uint64 hash_bytes_uint32_extended(uint32 k, uint64 seed); + +#ifndef FRONTEND .. Wrapper functions .. +#endif + +extern uint32 hash_bytes(const unsigned char *k, int keylen); +extern uint64 hash_bytes_extended(const unsigned char *k, + int keylen, uint64 seed); +extern uint32 hash_bytes_uint32(uint32 k); +extern uint64 hash_bytes_uint32_extended(uint32 k, uint64 seed); 3: The first line of the commit message has one typo. defiend => defined. On Fri, Feb 7, 2020 at 11:00 PM Robert Haas wrote: > Late last year, I did some work to make it possible to use simplehash > in frontend code.[1] However, a hash table is not much good unless one > also has some hash functions that one can use to hash the keys that > need to be inserted into that hash table. I initially thought that > solving this problem was going to be pretty annoying, but when I > looked at it again today I found what I think is a pretty simple way > to adapt things so that the hashing routines we use in the backend are > easily available to frontend code. > > Here are some patches for that. It may make sense to combine some of > these in terms of actually getting this committed, but I have > separated them here so that it is, hopefully, easy to see what I did > and why I did it. There are three basic problems which are solved by > the three preparatory patches: > > 0001 - hashfn.c has a couple of routines that depend on bitmapsets, > and bitmapset.c is currently backend-only. Fix by moving this code > near related code in bitmapset.c. > > 0002 - some of the prototypes for functions in hashfn.c are in > hsearch.h, mixed with the dynahash stuff, and others are in > hashutils.c. Fix by making hashutils.h the one true header for > hashfn.c. > > 0003 - Some of hashfn.c's routines return Datum, but that's > backend-only. Fix by renaming the functions and changing the return > types to uint32 and uint64, and add static inline wrappers with the > old names that convert to Datum. Just changing the return types of the > existing functions seemed like it would've required a lot more code > churn, and also seems like it could cause silent breakage in the > future. > > Thanks, > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > [1] > http://postgr.es/m/CA+Tgmob8oyh02NrZW=xCScB+5GyJ-jVowE3+TWTUmPF=fsg...@mail.gmail.com > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: backup manifests
Thank you for review comments.
On Mon, Dec 30, 2019 at 11:53 PM Robert Haas wrote:
> On Tue, Dec 24, 2019 at 5:42 AM Suraj Kharage
> wrote:
> > To examine the first word of each line, I am using below check:
> > if (strncmp(line, "File", 4) == 0)
> > {
> > ..
> > }
> > else if (strncmp(line, "Manifest-Checksum", 17) == 0)
> > {
> > ..
> > }
> > else
> > error
> >
> > strncmp might be not right here, but we can not put '\0' in between the
> line (to find out first word)
> > before we recognize the line type.
> > All the lines expect line last one (where we have manifest checksum) are
> feed to the checksum machinary to calculate manifest checksum.
> > so update_checksum() should be called after recognizing the type, i.e:
> if it is a File type record. Do you see any issues with this?
>
> I see the problem, but I don't think your solution is right, because
> the first test would pass if the line said FiletMignon rather than
> just File, which we certainly don't want. You've got to write the test
> so that you're checking against the whole first word, not just some
> prefix of it. There are several possible ways to accomplish that, but
> this isn't one of them.
>
Yeah. Fixed in the attached patch.
>
> >> + pg_log_error("invalid record found in \"%s\"", manifest_path);
> >>
> >> Error message needs work.
>
> Looks better now, but you have a messages that say "invalid checksums
> type \"%s\" found in \"%s\"". This is wrong because checksums would
> need to be singular in this context (checksum). Also, I think it could
> be better phrased as "manifest file \"%s\" specifies unknown checksum
> algorithm \"%s\" at line %d".
>
Corrected.
>
> >> Your function names should be consistent with the surrounding style,
> >> and with each other, as far as possible. Three different conventions
> >> within the same patch and source file seems over the top.
>
> This appears to be fixed.
>
> >> Also keep in mind that you're not writing code in a vacuum. There's a
> >> whole file of code here, and around that, a whole project.
> >> scan_data_directory() is a good example of a function whose name is
> >> clearly too generic. It's not a general-purpose function for scanning
> >> the data directory; it's specifically a support function for verifying
> >> a backup. Yet, the name gives no hint of this.
>
> But this appears not to be fixed.
>
I have changed this function name to "VerifyDir" likewise, we have sendDir
and sendFile in basebackup.c
>
> >> if (strcmp(newpathsuffix, "/pg_wal") == 0 || strcmp(newpathsuffix,
> >> "/backup_manifest") == 0)
> >> continue;
> >
> > Thanks for the suggestion. Corrected as per the above inputs.
>
> You need a comment here, like "Ignore the possible presence of a
> backup_manifest file and/or a pg_wal directory in the backup being
> verified." and then maybe another sentence explaining why that's the
> right thing to do.
>
Corrected.
>
> + * The forth parameter to VerifyFile() will pass the relative
> path
> + * of file to match exactly with the filename present in
> manifest.
>
> I don't know what this comment is trying to tell me, which might be
> something you want to try to fix. However, I'm pretty sure it's
> supposed to say "fourth" not "forth".
>
I have changed the fourth parameter of VerifyFile(), so my comment over
there is no more valid.
>
> >> and the result would be that everything inside that long if-block is
> >> now at the top level of the function and indented one level less. And
> >> I think if you look at this function you'll see a way that you can
> >> save a *second* level of indentation for much of that code. Please
> >> check the rest of the patch for similar cases, too.
> >
> > Make sense. corrected.
>
> I don't agree. A large chunk of VerifyFile() is still subject to a
> quite unnecessary level of indentation.
>
Yeah, corrected.
>
> > I have added a check for EOF, but not sure whether that woule be right
> here.
> > Do we need to check the length of buffer as well?
>
> That's really, really not right. EOF is not a character that can
> appear in the buffer. It's chosen on purpose to be a value that never
> matches any actual character when both the character and the EOF value
> are regarded as values of type 'int'. That guarantee doesn't apply
> here though because you're dealing with v
Re: backup manifests
Thank you for review comments.
On Fri, Dec 20, 2019 at 9:14 PM Robert Haas wrote:
> On Fri, Dec 20, 2019 at 8:24 AM Suraj Kharage
> wrote:
> > Thank you for review comments.
>
> Thanks for the new version.
>
> + --verify-backup
>
> Whitespace.
>
Corrected.
>
> +struct manifesthash_hash *hashtab;
>
> Uh, I had it in mind that you would nuke this line completely, not
> just remove "typedef" from it. You shouldn't need a global variable
> here.
>
Removed.
> + if (buf == NULL)
>
> pg_malloc seems to have an internal check such that it never returns
> NULL. I don't see anything like this test in other callers.
>
Yeah, removed this check
>
> The order of operations in create_manifest_hash() seems unusual:
>
> + fd = open(manifest_path, O_RDONLY, 0);
> + if (fstat(fd, ))
> + buf = pg_malloc(stat.st_size);
> + hashtab = manifesthash_create(1024, NULL);
> ...
> + entry = manifesthash_insert(hashtab, filename, );
> ...
> + close(fd);
>
> I would have expected open-fstat-read-close to be consecutive, and the
> manifesthash stuff all done afterwards. In fact, it seems like reading
> the file could be a separate function.
>
Yes, created new function which will read the file and return the buffer.
>
> + if (strncmp(checksum, "SHA256", 6) == 0)
>
> This isn't really right; it would give a false match if we had a
> checksum algorithm with a name like SHA2560 or SHA256C or
> SHA256ExceptWayBetter. The right thing to do is find the colon first,
> and then probably overwrite it with '\0' so that you have a string
> that you can pass to parse_checksum_algorithm().
>
Corrected this check. Below suggestion, allow us to put '\0' in between the
line.
since SHA256 is used to generate for backup manifest, so that we can feed
that
line early to the checksum machinery.
>
> + /*
> + * we don't have checksum type in the header, so need to
> + * read through the first file enttry to find the checksum
> + * type for the manifest file and initilize the checksum
> + * for the manifest file itself.
> + */
>
> This seems to be proceeding on the assumption that the checksum type
> for the manifest itself will always be the same as the checksum type
> for the first file in the manifest. I don't think that's the right
> approach. I think the manifest should always have a SHA256 checksum,
> regardless of what type of checksum is used for the individual files
> within the manifest. Since the volume of data in the manifest is
> presumably very small compared to the size of the database cluster
> itself, I don't think there should be any performance problem there.
>
Made the change in backup manifest as well in backup validatort patch.
Thanks to Rushabh Lathia for the offline discussion and help.
To examine the first word of each line, I am using below check:
if (strncmp(line, "File", 4) == 0)
{
..
}
else if (strncmp(line, "Manifest-Checksum", 17) == 0)
{
..
}
else
error
strncmp might be not right here, but we can not put '\0' in between the
line (to find out first word)
before we recognize the line type.
All the lines expect line last one (where we have manifest checksum) are
feed to the checksum machinary to calculate manifest checksum.
so update_checksum() should be called after recognizing the type, i.e: if
it is a File type record. Do you see any issues with this?
+ filesize = atol(size);
>
> Using strtol() would allow for some error checking.
>
corrected.
>
> + * Increase the checksum by its lable length so that we can
> + checksum = checksum + checksum_lable_length;
>
> Spelling.
>
corrected.
>
> + pg_log_error("invalid record found in \"%s\"", manifest_path);
>
> Error message needs work.
>
> +VerifyBackup(void)
> +create_manifest_hash(char *manifest_path)
> +nextLine(char *buf)
>
> Your function names should be consistent with the surrounding style,
> and with each other, as far as possible. Three different conventions
> within the same patch and source file seems over the top.
>
> Also keep in mind that you're not writing code in a vacuum. There's a
> whole file of code here, and around that, a whole project.
> scan_data_directory() is a good example of a function whose name is
> clearly too generic. It's not a general-purpose function for scanning
> the data directory; it's specifically a support function for verifying
> a backup. Yet, the name gives no hint of this.
>
> +verify_file(struct dirent *de, char fn[MAXPGPATH], struct stat st,
> + char relative_path[MAXPGPATH], manifesthash_hash *hashtab)
>
> I think I commented on the use of char[] parameters in my previous review.
>
> + /* Skip backup manifest file. */
> + if (strcmp(de->
Re: backup manifests
Fixed some typos in attached v5-0002 patch. Please consider this patch for
review.
On Fri, Dec 20, 2019 at 6:54 PM Suraj Kharage <
suraj.khar...@enterprisedb.com> wrote:
> Thank you for review comments.
>
> On Thu, Dec 19, 2019 at 2:54 AM Robert Haas wrote:
>
>> On Tue, Dec 17, 2019 at 12:54 AM Suraj Kharage
>> wrote:
>> > I have implemented the simplehash in backup validator patch as Robert
>> suggested. Please find attached 0002 patch for the same.
>> >
>> > kindly review and let me know your thoughts.
>>
>> +#define CHECKSUM_LENGTH 256
>>
>> This seems wrong. Not all checksums are the same length, and none of
>> the ones we're using are 256 bytes in length, and if we've got to have
>> a constant someplace for the maximum checksum length, it should
>> probably be in the new header file, not here. But I don't think we
>> should need this in the first place; see comments below about how to
>> revise the parsing of the manifest file.
>>
>
> I agree. Removed.
>
> +charfiletype[10];
>>
>> A mysterious 10-byte field with no comments explaining what it
>> means... and the same magic number 10 appears in at least one other
>> place in the patch.
>>
>
> with current logic, we don't need this anymore.
> I have removed the filetype from the structure as we are not doing any
> comparison anywhere.
>
>
>>
>> +typedef struct manifesthash_hash *hashtab;
>>
>> This declares a new *type* called hashtab, not a variable called
>> hashtab. The new type is not used anywhere, but later, you have
>> several variables of the same type that have this name. Just remove
>> this: it's wrong and unused.
>>
>>
> corrected.
>
>
>> +static enum ChecksumAlgorithm checksum_type = MC_NONE;
>>
>> Remove "enum". Not needed, because you have a typedef for it in the
>> header, and not per style.
>>
>> corrected.
>
>
>> +static manifesthash_hash *create_manifest_hash(char
>> manifest_path[MAXPGPATH]);
>>
>> Whitespace is wrong. The whole patch needs a visit from pgindent with
>> a properly-updated typedefs.list.
>>
>> Also, you will struggle to find anywhere else in the code base where
>> pass a character array as a function argument. I don't know why this
>> isn't just char *.
>>
>
> Corrected.
>
>
>>
>> +if(verify_backup)
>>
>> Whitespace wrong here, too.
>>
>>
> Fixed
>
>
>>
>> It's also pretty unhelpful, here and elsewhere, to refer to "the hash
>> table" as if there were only one, and as if the reader were supposed
>> to know something about it when you haven't told them anything about
>> it.
>>
>> +if (!entry->matched)
>> +{
>> +pg_log_info("missing file: %s", entry->filename);
>> +}
>> +
>>
>> The braces here are not project style. We usually omit braces when
>> only a single line of code is present.
>>
>
> fixed
>
>
>>
>> I think some work needs to be done to standardize and improve the
>> messages that get produced here. You have:
>>
>> 1. missing file: %s
>> 2. duplicate file present: %s
>> 3. size changed for file: %s, original size: %d, current size: %zu
>> 4. checksum difference for file: %s
>> 5. extra file found: %s
>>
>> I suggest:
>>
>> 1. file \"%s\" is present in manifest but missing from the backup
>> 2. file \"%s\" has multiple manifest entries
>> (this one should probably be pg_log_error(), not pg_log_info(), as it
>> represents a corrupt-manifest problem)
>> 3. file \"%s" has size %lu in manifest but size %lu in backup
>> 4. file \"%s" has checksum %s in manifest but checksum %s in backup
>> 5. file \"%s" is present in backup but not in manifest
>>
>
> Corrected.
>
>
>>
>> Your patch actually doesn't compile on my system, because for the
>> third message above, it uses %zu to print the size. But %zu is for
>> size_t, not off_t. I went looking for other places in the code where
>> we print off_t; based on that, I think the right thing to do is to
>> print it using %lu and write (unsigned long) st.st_size.
>>
>
> Corrected.
>
> +charfile_checksum[256];
>> +charheader[1024];
>>
>> More arbitrary constants.
>
>
>
>>
>> +if (!file)
>> +{
>> +pg_log_error("could not open backup_manifest");
>>
Re: backup manifests
Thank you for review comments.
On Thu, Dec 19, 2019 at 2:54 AM Robert Haas wrote:
> On Tue, Dec 17, 2019 at 12:54 AM Suraj Kharage
> wrote:
> > I have implemented the simplehash in backup validator patch as Robert
> suggested. Please find attached 0002 patch for the same.
> >
> > kindly review and let me know your thoughts.
>
> +#define CHECKSUM_LENGTH 256
>
> This seems wrong. Not all checksums are the same length, and none of
> the ones we're using are 256 bytes in length, and if we've got to have
> a constant someplace for the maximum checksum length, it should
> probably be in the new header file, not here. But I don't think we
> should need this in the first place; see comments below about how to
> revise the parsing of the manifest file.
>
I agree. Removed.
+charfiletype[10];
>
> A mysterious 10-byte field with no comments explaining what it
> means... and the same magic number 10 appears in at least one other
> place in the patch.
>
with current logic, we don't need this anymore.
I have removed the filetype from the structure as we are not doing any
comparison anywhere.
>
> +typedef struct manifesthash_hash *hashtab;
>
> This declares a new *type* called hashtab, not a variable called
> hashtab. The new type is not used anywhere, but later, you have
> several variables of the same type that have this name. Just remove
> this: it's wrong and unused.
>
>
corrected.
> +static enum ChecksumAlgorithm checksum_type = MC_NONE;
>
> Remove "enum". Not needed, because you have a typedef for it in the
> header, and not per style.
>
> corrected.
> +static manifesthash_hash *create_manifest_hash(char
> manifest_path[MAXPGPATH]);
>
> Whitespace is wrong. The whole patch needs a visit from pgindent with
> a properly-updated typedefs.list.
>
> Also, you will struggle to find anywhere else in the code base where
> pass a character array as a function argument. I don't know why this
> isn't just char *.
>
Corrected.
>
> +if(verify_backup)
>
> Whitespace wrong here, too.
>
>
Fixed
>
> It's also pretty unhelpful, here and elsewhere, to refer to "the hash
> table" as if there were only one, and as if the reader were supposed
> to know something about it when you haven't told them anything about
> it.
>
> +if (!entry->matched)
> +{
> +pg_log_info("missing file: %s", entry->filename);
> +}
> +
>
> The braces here are not project style. We usually omit braces when
> only a single line of code is present.
>
fixed
>
> I think some work needs to be done to standardize and improve the
> messages that get produced here. You have:
>
> 1. missing file: %s
> 2. duplicate file present: %s
> 3. size changed for file: %s, original size: %d, current size: %zu
> 4. checksum difference for file: %s
> 5. extra file found: %s
>
> I suggest:
>
> 1. file \"%s\" is present in manifest but missing from the backup
> 2. file \"%s\" has multiple manifest entries
> (this one should probably be pg_log_error(), not pg_log_info(), as it
> represents a corrupt-manifest problem)
> 3. file \"%s" has size %lu in manifest but size %lu in backup
> 4. file \"%s" has checksum %s in manifest but checksum %s in backup
> 5. file \"%s" is present in backup but not in manifest
>
Corrected.
>
> Your patch actually doesn't compile on my system, because for the
> third message above, it uses %zu to print the size. But %zu is for
> size_t, not off_t. I went looking for other places in the code where
> we print off_t; based on that, I think the right thing to do is to
> print it using %lu and write (unsigned long) st.st_size.
>
Corrected.
+charfile_checksum[256];
> +charheader[1024];
>
> More arbitrary constants.
>
> +if (!file)
> +{
> +pg_log_error("could not open backup_manifest");
>
> That's bad error reporting. See e.g. readfile() in initdb.c.
>
Corrected.
>
> +if (fscanf(file, "%1023[^\n]\n", header) != 1)
> +{
> +pg_log_error("error while reading the header from
> backup_manifest");
>
> That's also bad error reporting. It is only a slight step up from
> "ERROR: error".
>
> And we have another magic number (1023).
>
With current logic, we don't need this anymore.
>
> +appendPQExpBufferStr(manifest, header);
> +appendPQExpBufferStr(manifest, "\n");
> ...
> +appendPQExpBuffer(manifest, "File\t%s\t%d\t%s\t%s\n", filename,
> + filesize, mtime, checksum_with_type);
>
&g
Re: backup manifests
Hi, > I think that what we should actually do here is try to use simplehash. >> Right now, it won't work for frontend code, but I posted some patches >> to try to address that issue: >> >> >> https://www.postgresql.org/message-id/CA%2BTgmob8oyh02NrZW%3DxCScB%2B5GyJ-jVowE3%2BTWTUmPF%3DFsGWTA%40mail.gmail.com >> >> That would have a few advantages. One, we wouldn't need to know the >> number of elements in advance, because simplehash can grow >> dynamically. Two, we could use the iteration interfaces to walk the >> hash table. Your solution to that is pgrhash_seq_search, but that's >> actually not well-designed, because it's not a generic iterator >> function but something that knows specifically about the 'touch' flag. >> I incidentally suggest renaming 'touch' to 'matched;' 'touch' is not >> bad, but I think 'matched' will be a little more recognizable. >> > > Thanks for the suggestion. Will try to implement the same and update > accordingly. > I am assuming that I need to build the patch based on the changes that you > proposed on the mentioned thread. > > I have implemented the simplehash in backup validator patch as Robert suggested. Please find attached 0002 patch for the same. kindly review and let me know your thoughts. Also attached the remaining patches. 0001 and 0003 are same as v2, only patch version is bumped. -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company. v3-0001-Backup-manifest-with-file-names-sizes-timestamps-.patch Description: Binary data v3-0002-Implementation-of-backup-validator.patch Description: Binary data v3-0003-Tap-test-case-patch-to-verify-the-backup-using-ve.patch Description: Binary data
Re: backup manifests
Thanks, Robert for the review. On Wed, Dec 11, 2019 at 1:10 AM Robert Haas wrote: > On Tue, Dec 10, 2019 at 6:40 AM Suraj Kharage > wrote: > > Please find attached patch for backup validator implementation (0004 > patch). This patch is based > > on Rushabh's latest patch for backup manifest. > > > > There are some functions required at client side as well, so I have > moved those functions > > and some data structure at common place so that they can be accessible > for both. (0003 patch). > > > > My colleague Rajkumar Raghuwanshi has prepared the WIP patch (0005) for > tap test cases which > > is also attached. As of now, test cases related to the tablespace and > tar backup format are missing, > > will continue work on same and submit the complete patch. > > > > With this mail, I have attached the complete patch stack for backup > manifest and backup > > validate implementation. > > > > Please let me know your thoughts on the same. > > Well, for the second time on this thread, please don't take a bunch of > somebody else's code and post it in a patch that doesn't attribute > that person as one of the authors. For the second time on this thread, > the person is me, but don't borrow *anyone's* code without proper > attribution. It's really important! > > On a related note, it's a very good idea to use git format-patch and > git rebase -i to maintain patch stacks like this. Rushabh seems to > have done that, but the files you're posting look like raw 'git diff' > output. Notice that this gives him a way to include authorship > information and a tentative commit message in each patch, but you > don't have any of that. > Sorry, I have corrected this in the attached v2 patch set. > Also on a related note, part of the process of adapting existing code > to a new purpose is adapting the comments. You haven't done that: > > + * Search a result-set hash table for a row matching a given filename. > ... > + * Insert a row into a result-set hash table, provided no such row is > already > ... > + * Most of the values > + * that we're hashing are short integers formatted as text, so there > + * shouldn't be much room for pathological input. > Corrected in v2 patch. > I think that what we should actually do here is try to use simplehash. > Right now, it won't work for frontend code, but I posted some patches > to try to address that issue: > > > https://www.postgresql.org/message-id/CA%2BTgmob8oyh02NrZW%3DxCScB%2B5GyJ-jVowE3%2BTWTUmPF%3DFsGWTA%40mail.gmail.com > > That would have a few advantages. One, we wouldn't need to know the > number of elements in advance, because simplehash can grow > dynamically. Two, we could use the iteration interfaces to walk the > hash table. Your solution to that is pgrhash_seq_search, but that's > actually not well-designed, because it's not a generic iterator > function but something that knows specifically about the 'touch' flag. > I incidentally suggest renaming 'touch' to 'matched;' 'touch' is not > bad, but I think 'matched' will be a little more recognizable. > Thanks for the suggestion. Will try to implement the same and update accordingly. I am assuming that I need to build the patch based on the changes that you proposed on the mentioned thread. > Please run pgindent. If needed, first add locally defined types to > typedefs.list, so that things indent properly. > > It's not a crazy idea to try to share some data structures and code > between the frontend and the backend here, but I think > src/common/backup.c and src/include/common/backup.h is a far too > generic name given what the code is actually doing. It's mostly about > checksums, not backup, and I think it should be named accordingly. I > suggest removing "manifestinfo" and renaming the rest to just talk > about checksums rather than manifests. That would make it logical to > reuse this for any other future code that needs a configurable > checksum type. Also, how about adding a function like: > > extern bool parse_checksum_algorithm(char *name, ChecksumAlgorithm *algo); > > ...which would return true and set *algo if name is recognized, and > return false otherwise. That code could be used on both the client and > server sides of this patch, and by any future patches that want to > return this scaffolding. > Corrected the filename and implemented the function as suggested. > The file header for backup.h has the wrong filename (string.h). The > header format looks somewhat atypical compared to what we normally do, > too. My bad, corrected the header format as well. > > It's arguable, but I tend to think that it would be better to > hex-encode the CRC rather than printing it as an in
Re: backup manifests
Hi, Please find attached patch for backup validator implementation (0004 patch). This patch is based on Rushabh's latest patch for backup manifest. There are some functions required at client side as well, so I have moved those functions and some data structure at common place so that they can be accessible for both. (0003 patch). My colleague Rajkumar Raghuwanshi has prepared the WIP patch (0005) for tap test cases which is also attached. As of now, test cases related to the tablespace and tar backup format are missing, will continue work on same and submit the complete patch. With this mail, I have attached the complete patch stack for backup manifest and backup validate implementation. Please let me know your thoughts on the same. On Fri, Dec 6, 2019 at 1:44 AM Robert Haas wrote: > On Thu, Dec 5, 2019 at 11:22 AM Rushabh Lathia > wrote: > > Here is the whole stack of patches. > > I committed 0001, as that's just refactoring and I think (hope) it's > uncontroversial. I think 0002-0005 need to be squashed together > (crediting all authors properly and in the appropriate order) as it's > quite hard to understand right now, and that Suraj's patch to validate > the backup should be included in the patch stack. It needs > documentation. Also, we need, either in that patch or a separate, TAP > tests that exercise this feature. Things we should try to check: > > - Plain format backups can be verified against the manifest. > - Tar format backups can be verified against the manifest after > untarring (this might be a problem; not sure there's any guarantee > that we have a working "tar" command available). > - Verification succeeds for all available checksums algorithms and > also for no checksum algorithm (should still check which files are > present, and sizes). > - If we tamper with a backup by removing a file, adding a file, or > changing the size of a file, the modification is detected even without > checksums. > - If we tamper with a backup by changing the contents of a file but > not the size, the modification is detected if checksums are used. > - Everything above still works if there is user-defined tablespace > that contains a table. > > -- > Robert Haas > EnterpriseDB: http://www.enterprisedb.com > The Enterprise PostgreSQL Company > > > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company. 0001-Backup-manifest-with-file-names-sizes-timestamps-opt.patch Description: Binary data 0002_new_data_structure_v2.patch Description: Binary data 0003-refactor.patch Description: Binary data 0004-backup-validator.patch Description: Binary data 0005-backup_validator_tap_test_WIP.patch Description: Binary data
Re: backup manifests
Hi Jeevan,
I have incorporated all the comments in the attached patch. Please review
and let me know your thoughts.
On Thu, Nov 21, 2019 at 2:51 PM Jeevan Chalke <
jeevan.cha...@enterprisedb.com> wrote:
>
>
> On Wed, Nov 20, 2019 at 11:05 AM Suraj Kharage <
> suraj.khar...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> Since now we are generating the backup manifest file with each backup, it
>> provides us an option to validate the given backup.
>> Let's say, we have taken a backup and after a few days, we want to check
>> whether that backup is validated or corruption-free without restarting the
>> server.
>>
>> Please find attached POC patch for same which will be based on the latest
>> backup manifest patch from Rushabh. With this functionality, we add new
>> option to pg_basebackup, something like --verify-backup.
>> So, the syntax would be:
>> ./bin/pg_basebackup --verify-backup -D
>>
>> Basically, we read the backup_manifest file line by line from the given
>> directory path and build the hash table, then scan the directory and
>> compare each file with the hash entry.
>>
>> Thoughts/suggestions?
>>
>
>
> I like the idea of verifying the backup once we have backup_manifest with
> us.
> Periodically verifying the already taken backup with this simple tool
> becomes
> easy now.
>
> I have reviewed this patch and here are my comments:
>
> 1.
> @@ -30,7 +30,9 @@
> #include "common/file_perm.h"
> #include "common/file_utils.h"
> #include "common/logging.h"
> +#include "common/sha2.h"
> #include "common/string.h"
> +#include "fe_utils/simple_list.h"
> #include "fe_utils/recovery_gen.h"
> #include "fe_utils/string_utils.h"
> #include "getopt_long.h"
> @@ -38,12 +40,19 @@
> #include "pgtar.h"
> #include "pgtime.h"
> #include "pqexpbuffer.h"
> +#include "pgrhash.h"
> #include "receivelog.h"
> #include "replication/basebackup.h"
> #include "streamutil.h"
>
> Please add new files in order.
>
> 2.
> Can hash related file names be renamed to backuphash.c and backuphash.h?
>
> 3.
> Need indentation adjustments at various places.
>
> 4.
> +charbuf[100]; // 1MB chunk
>
> It will be good if we have multiple of block /page size (or at-least power
> of 2
> number).
>
> 5.
> +typedef struct pgrhash_entry
> +{
> +struct pgrhash_entry *next; /* link to next entry in same bucket */
> +DataDirectoryFileInfo *record;
> +} pgrhash_entry;
> +
> +struct pgrhash
> +{
> +unsignednbuckets;/* number of buckets */
> +pgrhash_entry **bucket;/* pointer to hash entries */
> +};
> +
> +typedef struct pgrhash pgrhash;
>
> These two can be moved to .h file instead of redefining over there.
>
> 6.
> +/*
> + * TODO: this function is not necessary, can be removed.
> + * Test whether the given row number is match for the supplied keys.
> + */
> +static bool
> +pgrhash_compare(char *bt_filename, char *filename)
>
> Yeah, it can be removed by doing strcmp() at the required places rather
> than
> doing it in a separate function.
>
> 7.
> mdate is not compared anywhere. I understand that it can't be compared with
> the file in the backup directory and its entry in the manifest as manifest
> entry gives mtime from server file whereas the same file in the backup will
> have different mtime. But adding a few comments there will be good.
>
> 8.
> +charmdate[24];
>
> should be mtime instead?
>
>
> Thanks
>
> --
> Jeevan Chalke
> Associate Database Architect & Team Lead, Product Development
> EnterpriseDB Corporation
> The Enterprise PostgreSQL Company
>
>
--
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
backup_validator_POC.patch
Description: Binary data
Re: backup manifests
Hi, Since now we are generating the backup manifest file with each backup, it provides us an option to validate the given backup. Let's say, we have taken a backup and after a few days, we want to check whether that backup is validated or corruption-free without restarting the server. Please find attached POC patch for same which will be based on the latest backup manifest patch from Rushabh. With this functionality, we add new option to pg_basebackup, something like --verify-backup. So, the syntax would be: ./bin/pg_basebackup --verify-backup -D Basically, we read the backup_manifest file line by line from the given directory path and build the hash table, then scan the directory and compare each file with the hash entry. Thoughts/suggestions? On Tue, Nov 19, 2019 at 3:30 PM Rushabh Lathia wrote: > > > My colleague Suraj did testing and noticed the performance impact > with the checksums. On further testing, he found that specifically with > sha its more of performance impact. > > Please find below statistics: > > no of tables without checksum SHA256 > checksum % performnce > overhead > with > SHA-256 md5 checksum % performnce > overhead with md5 CRC checksum % performnce > overhead with > CRC > 10 (100 MB > in each table) real 0m10.957s > user 0m0.367s > sys 0m2.275s real 0m16.816s > user 0m0.210s > sys 0m2.067s 53% real 0m11.895s > user 0m0.174s > sys 0m1.725s 8% real 0m11.136s > user 0m0.365s > sys 0m2.298s 2% > 20 (100 MB > in each table) real 0m20.610s > user 0m0.484s > sys 0m3.198s real 0m31.745s > user 0m0.569s > sys 0m4.089s > 54% real 0m22.717s > user 0m0.638s > sys 0m4.026s 10% real 0m21.075s > user 0m0.538s > sys 0m3.417s 2% > 50 (100 MB > in each table) real 0m49.143s > user 0m1.646s > sys 0m8.499s real 1m13.683s > user 0m1.305s > sys 0m10.541s 50% real 0m51.856s > user 0m0.932s > sys 0m7.702s 6% real 0m49.689s > user 0m1.028s > sys 0m6.921s 1% > 100 (100 MB > in each table) real 1m34.308s > user 0m2.265s > sys 0m14.717s real 2m22.403s > user 0m2.613s > sys 0m20.776s 51% real 1m41.524s > user 0m2.158s > sys 0m15.949s > 8% real 1m35.045s > user 0m2.061s > sys 0m16.308s 1% > 100 (1 GB > in each table) real 17m18.336s > user 0m20.222s > sys 3m12.960s real 24m45.942s > user 0m26.911s > sys 3m33.501s 43% real 17m41.670s > user 0m26.506s > sys 3m18.402s 2% real 17m22.296s > user 0m26.811s > sys 3m56.653s > > sometimes, this test > completes within the > same time as without > checksum. approx. 0.5% > > > Considering the above results, I modified the earlier Robert's patch and > added > "manifest_with_checksums" option to pg_basebackup. With a new patch. > by default, checksums will be disabled and will be only enabled when > "manifest_with_checksums" option is provided. Also re-based all patch set. > > > > Regards, > > -- > Rushabh Lathia > www.EnterpriseDB.com > > On Tue, Oct 1, 2019 at 5:43 PM Robert Haas wrote: > >> On Mon, Sep 30, 2019 at 5:31 AM Jeevan Chalke >> wrote: >> > Entry for directory is not added in manifest. So it might be difficult >> > at client to get to know about the directories. Will it be good to add >> > an entry for each directory too? May be like: >> > Dir >> >> Well, what kind of corruption would this allow us to detect that we >> can't detect as things stand? I think the only case is an empty >> directory. If it's not empty, we'd have some entries for the files in >> that directory, and those files won't be able to exist unless the >> directory does. But, how would we end up backing up an empty >> directory, anyway? >> >> I don't really *mind* adding directories into the manifest, but I'm >> not sure how much it helps. >> >> -- >> Robert Haas >> EnterpriseDB: http://www.enterprisedb.com >> The Enterprise PostgreSQL Company >> >> >> > > -- > Rushabh Lathia > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company. backup_validator_POC.patch Description: Binary data
Re: identity column behavior in WHEN condition for BEFORE EACH ROW trigger
> whereas, for identity columns, server allows us to create trigger for same > and trigger gets invoked as defined. Is this behavior expected? or we need > to restrict the identity columns in such scenario because anyone one > override the identity column value in trigger. > Also, I think it is breaking the OVERRIDING SYSTEM VALUE clause in INSERT statement. i.e: without this clause, can insert the modified value from trigger in identity column. I don't find any document reference for this behavior. Thoughts? -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
identity column behavior in WHEN condition for BEFORE EACH ROW trigger
Hi, It is been observed that when we define the generated columns in WHEN condition for BEFORE EACH ROW trigger then server throw an error from CreateTrigger(). e.g: create table bar(a int PRIMARY KEY, b int GENERATED ALWAYS AS (a * 2) STORED); CREATE OR REPLACE FUNCTION test() RETURNS trigger AS $$ BEGIN NEW.b = 10; raise notice 'Before row trigger'; RETURN NEW; END; $$ LANGUAGE plpgsql; postgres@78049=#CREATE TRIGGER bar_trigger BEFORE INSERT ON bar FOR EACH ROW *WHEN (NEW.b < 8)* EXECUTE FUNCTION test(); 2019-10-03 19:25:29.945 IST [78049] ERROR: *BEFORE trigger's WHEN condition cannot reference NEW generated columns* at character 68 2019-10-03 19:25:29.945 IST [78049] DETAIL: Column "b" is a generated column. 2019-10-03 19:25:29.945 IST [78049] STATEMENT: CREATE TRIGGER bar_trigger BEFORE INSERT ON bar FOR EACH ROW WHEN (NEW.b < 8) EXECUTE FUNCTION test(); ERROR: BEFORE trigger's WHEN condition cannot reference NEW generated columns LINE 4: WHEN (NEW.b < 8) ^ DETAIL: Column "b" is a generated column. whereas, for identity columns, server allows us to create trigger for same and trigger gets invoked as defined. Is this behavior expected? or we need to restrict the identity columns in such scenario because anyone one override the identity column value in trigger. e.g: create table foo(no int, id int generated always as identity); CREATE OR REPLACE FUNCTION test() RETURNS trigger AS $$ BEGIN NEW.id = 10; raise notice 'Before row trigger'; RETURN NEW; END; $$ LANGUAGE plpgsql; CREATE TRIGGER foo_trigger BEFORE INSERT ON foo FOR EACH ROW WHEN (NEW.id < 8) EXECUTE FUNCTION test(); postgres@78049=#insert into foo values(1); *NOTICE: Before row trigger* INSERT 0 1 postgres@78049=#select * from foo; no | id + 1 | *10* (1 row) Thoughts? -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company.
Re: Issue in to_timestamp/to_date while handling the quoted literal string
Thanks for the clarification Brendan, that really helps.
On Wed, Jul 24, 2019 at 6:24 PM Brendan Jurd wrote:
> Hi Suraj,
>
> I think the documentation is reasonably clear about this behaviour, quote:
>
> " In to_date, to_number, and to_timestamp, literal text and double-quoted
> strings result in skipping the number of characters contained in the
> string; for example "XX" skips two input characters (whether or not they
> are XX)."
>
> I can appreciate that this isn't the behaviour you intuitively expected
> from to_timestamp, and I don't think you'd be the first or the last. The
> purpose of these functions was never to validate that your input string
> precisely matches the non-coding parts of your format pattern. For that, I
> think you'd be better served by using regular expressions.
>
> Just as an aside, in the example you gave, the string
> '2019-05-24T23:12:45' will cast directly to timestamp just fine, so it
> isn't the kind of situation to_timestamp was really intended for. It's
> more for when your input string is in an obscure (or ambiguous) format that
> is known to you in advance.
>
> I hope that helps.
>
> Cheers
> Brendan
>
> On Wed, 24 Jul 2019 at 21:38, Suraj Kharage <
> suraj.khar...@enterprisedb.com> wrote:
>
>> Hi,
>>
>> I noticed the issue in to_timestamp()/to_date() while handling the double
>> quote literal string. If any double quote literal characters found in
>> format, we generate the NODE_TYPE_CHAR in parse format and store that
>> actual character in FormatNode->character. n DCH_from_char, we just
>> increment the input string by length of character for NODE_TYPE_CHAR.
>> We are actually not matching these characters in input string and because
>> of this, date values get changed if quoted literal string is not identical
>> in input and format string.
>>
>> e.g:
>>
>> postgres@78619=#select to_timestamp('2019-05-24T23:12:45',
>> '-mm-dd"TT"hh24:mi:ss');
>>to_timestamp
>> ---
>> 2019-05-24 03:12:45+05:30
>> (1 row)
>>
>>
>> In above example, the quoted string is 'TT', so it just increment the
>> input string by 2 while handling these characters and returned the wrong
>> hour value.
>>
>> My suggestion is to match the exact characters from quoted literal string
>> in input string and if doesn't match then throw an error.
>>
>> Attached is the POC patch which almost works for all scenarios except for
>> whitespace - as a quote character.
>>
>> Suggestions?
>> --
>> --
>>
>> Thanks & Regards,
>> Suraj kharage,
>> EnterpriseDB Corporation,
>> The Postgres Database Company.
>>
>
--
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
Issue in to_timestamp/to_date while handling the quoted literal string
Hi,
I noticed the issue in to_timestamp()/to_date() while handling the double
quote literal string. If any double quote literal characters found in
format, we generate the NODE_TYPE_CHAR in parse format and store that
actual character in FormatNode->character. n DCH_from_char, we just
increment the input string by length of character for NODE_TYPE_CHAR.
We are actually not matching these characters in input string and because
of this, date values get changed if quoted literal string is not identical
in input and format string.
e.g:
postgres@78619=#select to_timestamp('2019-05-24T23:12:45',
'-mm-dd"TT"hh24:mi:ss');
to_timestamp
---
2019-05-24 03:12:45+05:30
(1 row)
In above example, the quoted string is 'TT', so it just increment the input
string by 2 while handling these characters and returned the wrong hour
value.
My suggestion is to match the exact characters from quoted literal string
in input string and if doesn't match then throw an error.
Attached is the POC patch which almost works for all scenarios except for
whitespace - as a quote character.
Suggestions?
--
--
Thanks & Regards,
Suraj kharage,
EnterpriseDB Corporation,
The Postgres Database Company.
to_timestamp_quoted_string_POC.patch
Description: Binary data
Re: pg_dump/pg_restore fail for TAR_DUMP and CUSTOM_DUMP from v94/v95/v96 to v11/master.
Hi, The Commit 5955d934194c3888f30318209ade71b53d29777f has changed the logic to avoid dumping creation and comment commands for the public schema. >From v11 onwards, we are using the DUMP_COMPONENT_ infrastructure in selectDumpableNamespace() to skip the public schema creation. As reported by Prabhat, if we try to restore the custom/tar dump taken from v10 and earlier versions, we get the reported error for public schema. The reason for this error is, when we take custom/tar dump from v10 and earlier version, it has "CREATE SCHEMA public;" statement and v11 failed to bypass that as per the current logic. The plain format does not produces the error in this case, because in all versions, pg_dump in plain format does not generate that "CREATE SCHEMA public". In v10 and earlier, we filter out that public schema creation in _printTocEntry() while pg_dump. In custom/tar format, pg_dump in V10 and earlier versions generate the schema creation statement for public schema but again while pg_restore in same or back branches, it get skipped through same _printTocEntry() function. I think we can write a logic in - 1) BulidArchiveDependencies() to avoid dumping creation and comment commands for the public schema since we do not have DUMP_COMPONENT_ infrastructure in all supported back-branches. or 2) dumpNamespace() to not include public schema creation. Thoughts? Regards, Suraj
Re: Catalog views failed to show partitioned table information.
Thank you for review and commit. On Tue, Dec 18, 2018 at 1:12 PM Michael Paquier wrote: > On Mon, Dec 17, 2018 at 11:01:59AM +0900, Michael Paquier wrote: > > On Mon, Dec 17, 2018 at 10:22:28AM +0900, Amit Langote wrote: > >> On 2018/12/15 8:00, Michael Paquier wrote: > >>> I tend to agree with your comment here. pg_tables lists partitioned > >>> tables, but pg_indexes is forgotting about partitioned indexes. So > this > >>> is a good thing to add. > >> > >> +1 > > > > I'll go commit something close to what Suraj is proposing if there are > > no objections from others. At least we agree on that part ;) > > And this part is done. > -- > Michael > -- -- Thanks & Regards, Suraj kharage, EnterpriseDB Corporation, The Postgres Database Company. *Are you updated: Latest version of EnterpriseDB Postgres Advanced Server are 10.6.13, 9.6.11.18, 9.5.15.21, 9.4.19.28* To reach Support Call: US +1-732-331-1320 or 1-800-235-5891 UK +44-2033 7198 20 - BRAZIL+55-2129 5813 71 - INDIA+91-20-66449612 Australia: +61 26145 2339. Website: www.enterprisedb.com EnterpriseDB Blog : http://blogs.enterprisedb.com Follow us on Twitter : http://www.twitter.com/enterprisedb PRIVACY & CONFIDENTIALITY NOTICE This e-mail message (and any attachment) is intended for the use of the individual or entity to whom it is addressed. This message contains information from EnterpriseDB Corporation that may be privileged, confidential, or exempt from disclosure under applicable law. If you are not the intended recipient or authorized to receive this for the intended recipient, any use, dissemination, distribution,retention, archiving, or copying of this communication is strictly prohibited. If you have received this e-mail in error, please notify the sender immediately by reply e-mail and delete this message.
Catalog views failed to show partitioned table information.
Hi, There are some catalog views which do not show the partitioned table and its index entry. One of them is "pg_indexes" which failed to show the partitioned index. Attached the patch which fixes the same. Other views such as pg_stat*,pg_statio_* has the same problem for partitioned tables and indexes. Since the partitioned tables and its indexes considered as a dummy, they do not have any significance in stat tables, can we still consider adding relkind=p in these pg_stat_* views? Thoughts? Regards, Suraj pg_indexes_fix_for_partition_index.patch Description: Binary data
