Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oaIkm-0001vJ-RM for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Sep 2022 15:32:13 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1oaIkl-00006S-LN for pgsql-hackers@arkaria.postgresql.org; Mon, 19 Sep 2022 15:32:11 +0000 Received: from magus.postgresql.org ([2a02:c0:301:0:ffff::29]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1oaIkl-00006J-Ai for pgsql-hackers@lists.postgresql.org; Mon, 19 Sep 2022 15:32:11 +0000 Received: from mail-ua1-x92b.google.com ([2607:f8b0:4864:20::92b]) by magus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_128_GCM_SHA256:128) (Exim 4.92) (envelope-from ) id 1oaIki-0006iE-KX for pgsql-hackers@lists.postgresql.org; Mon, 19 Sep 2022 15:32:10 +0000 Received: by mail-ua1-x92b.google.com with SMTP id c23so132540uan.5 for ; Mon, 19 Sep 2022 08:32:08 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:from:to:cc:subject:date; bh=p+PJl9EkbH7dFh9qbjW0/24itSKwXEdRbmuFUpo0KUE=; b=N373JYQCrs6LfYDVadm4i/Er9YSCpN3fzAEQbdVP+/GWmB1NXKYybcr5iiIFsQlxyf ZOHxvqZp7OtG/0KT7cOinlJJYQibsfWjuktzXel/mt3s7Vq2qLzX8sCRKoumWZD+sQmV EcKS+jy590paLdUS7ovhVjPtOwe2E8wFZ7nLp2Rrt8safpxKVdyLHbQri4UuBUSLZj61 2FQ1Jr6sX895hEHxu11W0Ua64b7GxeAt9krF6tnoHN3w2qs7yn2YXRAZz3e7y+mhMihD 9dq7V7/+VirY4rqyD+2ygj+5Y0PLGaJGnCuWkcx2ea4lKcjl9ovhs9KseZ5wnbX/LaK1 fesw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20210112; h=cc:to:subject:message-id:date:from:in-reply-to:references :mime-version:x-gm-message-state:from:to:cc:subject:date; bh=p+PJl9EkbH7dFh9qbjW0/24itSKwXEdRbmuFUpo0KUE=; b=aWo+vg8xe8bg2Qe0U7r6EAwvZYbuRuKIh1cQqkI33lt7FpDQ2W3UV7Xh5JB3Kpt6ka eSZLoWfJ69WnMZjTKVWM2XYkf79XAyXGHsvZ0LdVRIp8oEbWvAUkkcGCqfRnN5R0TxEr Tm3lioj2sW6VxZmp9+j4DG4NoxZXlNMY4pYCmyqHfs3P1xxvt2zA1UikAZQR752KDuVe 9xA284rwD4zCacdcaXFEOnHblxtvsqshTfdIrDQwlz5xGbiWWjZfebhJu785WzX6+KDJ aabdxQwA8UzOi+355eAxybsfNVeaZStCPyilOiLUIKI9lA250fsME2Zv89zygAGoiIfo sd1g== X-Gm-Message-State: ACrzQf2bbUZ3HMhcQkWBIAISi0SBv4PLPhOAyA686dP9gMeV926FqQ8Z jlrrWyJVEiDZPAjFM0jdySwrhxNFPLgT/8ooSyx1/bn7E7M= X-Google-Smtp-Source: AMsMyM4qVjWqfTNz1pPww22VVfQxq3ku+F/5dlnsPgQ7+trMjQGk6RjxbrAw7eOurqbxwD1Fzrt5JgU5kAzHcRP7gFE= X-Received: by 2002:ab0:2106:0:b0:3bd:27cf:e95f with SMTP id d6-20020ab02106000000b003bd27cfe95fmr6392057ual.30.1663601527028; Mon, 19 Sep 2022 08:32:07 -0700 (PDT) MIME-Version: 1.0 References: In-Reply-To: From: =?UTF-8?B?w5ZuZGVyIEthbGFjxLE=?= Date: Mon, 19 Sep 2022 18:31:55 +0300 Message-ID: Subject: Re: [PATCH] Use indexes on the subscriber when REPLICA IDENTITY is full on the publisher To: "kuroda.hayato@fujitsu.com" Cc: PostgreSQL Hackers Content-Type: multipart/alternative; boundary="0000000000001101f305e9096a43" List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk --0000000000001101f305e9096a43 Content-Type: text/plain; charset="UTF-8" Hi Hayato Kuroda, Thanks for the review, please see my reply below: > === > For execRelation.c > > 01. RelationFindReplTupleByIndex() > > ``` > /* Start an index scan. */ > InitDirtySnapshot(snap); > - scan = index_beginscan(rel, idxrel, &snap, > - > IndexRelationGetNumberOfKeyAttributes(idxrel), > - 0); > > /* Build scan key. */ > - build_replindex_scan_key(skey, rel, idxrel, searchslot); > + scankey_attoff = build_replindex_scan_key(skey, rel, idxrel, > searchslot); > > + scan = index_beginscan(rel, idxrel, &snap, scankey_attoff, 0); > ``` > > I think "/* Start an index scan. */" should be just above > index_beginscan(). > moved there > > === > For worker.c > > 02. sable_indexoid_internal() > > ``` > + * Note that if the corresponding relmapentry has InvalidOid > usableIndexOid, > + * the function returns InvalidOid. > + */ > +static Oid > +usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo > *relinfo) > ``` > > "InvalidOid usableIndexOid" should be "invalid usableIndexOid," > makes sense, updated > > 03. check_relation_updatable() > > ``` > * We are in error mode so it's fine this is somewhat slow. It's > better to > * give user correct error. > */ > - if (OidIsValid(GetRelationIdentityOrPK(rel->localrel))) > + if (OidIsValid(rel->usableIndexOid)) > { > ``` > > Shouldn't we change the above comment to? The check is no longer slow. > Hmm, I couldn't realize this comment earlier. So you suggest "slow" here refers to the additional function call "GetRelationIdentityOrPK"? If so, yes I'll update that. > > === > For relation.c > > 04. GetCheapestReplicaIdentityFullPath() > > ``` > +static Path * > +GetCheapestReplicaIdentityFullPath(Relation localrel) > +{ > + PlannerInfo *root; > + Query *query; > + PlannerGlobal *glob; > + RangeTblEntry *rte; > + RelOptInfo *rel; > + int attno; > + RangeTblRef *rt; > + List *joinList; > + Path *seqScanPath; > ``` > > I think the part that constructs dummy-planner state can be move to > another function > because that part is not meaningful for this. > Especially line 824-846 can. > > Makes sense, simplified the function. Though, it is always hard to pick good names for these kinds of helper functions. I picked GenerateDummySelectPlannerInfoForRelation(), does that sound good to you as well? > > === > For 032_subscribe_use_index.pl > > 05. general > > ``` > +# insert some initial data within the range 0-1000 > +$node_publisher->safe_psql('postgres', > + "INSERT INTO test_replica_id_full SELECT i%20 FROM > generate_series(0,1000)i;" > +); > ``` > > It seems that the range of initial data seems [0, 19]. > Same mistake-candidates are found many place. > Ah, several copy & paste errors. Fixed (hopefully) all. > > 06. general > > ``` > +# updates 1000 rows > +$node_publisher->safe_psql('postgres', > + "UPDATE test_replica_id_full SET x = x + 1 WHERE x = 15;"); > ``` > > Only 50 tuples are modified here. > Same mistake-candidates are found many place. > Alright, yes there were several wrong comments in the tests. I went over the tests once more to fix those and improve comments. > > 07. general > > ``` > +# we check if the index is used or not > +$node_subscriber->poll_query_until( > + 'postgres', q{select (idx_scan = 200) from pg_stat_all_indexes > where indexrelname = 'test_replica_id_full_idx';} > +) or die "Timed out while waiting for check subscriber tap_sub_rep_full_3 > updates 200 rows via index"; > ``` > The query will be executed until the index scan is finished, but it may be > not commented. > How about changing it to "we wait until the index used on the > subscriber-side." or something? > Same comments are found in many place. > Makes sense, updated > > 08. test related with ANALYZE > > ``` > +# Testcase start: SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE > - PARTITIONED TABLE > +# ==================================================================== > ``` > > "Testcase start:" should be "Testcase end:" here. > thanks, fixed > > 09. general > > In some tests results are confirmed but in other test they are not. > I think you can make sure results are expected in any case if there are no > particular reasons. > > Alright, yes I also don't see a reason not to do that. Added to all cases. I'll attach the patch with the next email as I also want to incorporate the other comments. Hope this is not going to be confusing. Thanks, Onder --0000000000001101f305e9096a43 Content-Type: text/html; charset="UTF-8" Content-Transfer-Encoding: quoted-printable
Hi Hayato Kuroda,=C2=A0

Thanks for the review, please see my reply below:


=3D=3D=3D
For execRelation.c

01. RelationFindReplTupleByIndex()

```
=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Start an index scan. */
=C2=A0 =C2=A0 =C2=A0 =C2=A0 InitDirtySnapshot(snap);
-=C2=A0 =C2=A0 =C2=A0 =C2=A0scan =3D index_beginscan(rel, idxrel, &snap= ,
-=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 IndexRelationGetNumberOfKeyAttributes(idxrel),<= br> -=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2= =A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0 = =C2=A0 =C2=A0 =C2=A0 =C2=A0 0);

=C2=A0 =C2=A0 =C2=A0 =C2=A0 /* Build scan key. */
-=C2=A0 =C2=A0 =C2=A0 =C2=A0build_replindex_scan_key(skey, rel, idxrel, sea= rchslot);
+=C2=A0 =C2=A0 =C2=A0 =C2=A0scankey_attoff =3D build_replindex_scan_key(ske= y, rel, idxrel, searchslot);

+=C2=A0 =C2=A0 =C2=A0 =C2=A0scan =3D index_beginscan(rel, idxrel, &snap= , scankey_attoff, 0);
```

I think "/* Start an index scan. */" should be just above index_b= eginscan().

moved there
=C2= =A0

=3D=3D=3D
For worker.c

02. sable_indexoid_internal()

```
+ * Note that if the corresponding relmapentry has InvalidOid usableIndexOi= d,
+ * the function returns InvalidOid.
+ */
+static Oid
+usable_indexoid_internal(ApplyExecutionData *edata, ResultRelInfo *relinfo= )
```

"InvalidOid usableIndexOid" should be "invalid usableIndexOi= d,"

makes sense, updated
=C2=A0

03. check_relation_updatable()

```
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* We are in error mode so it's fine t= his is somewhat slow. It's better to
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0* give user correct error.
=C2=A0 =C2=A0 =C2=A0 =C2=A0 =C2=A0*/
-=C2=A0 =C2=A0 =C2=A0 =C2=A0if (OidIsValid(GetRelationIdentityOrPK(rel->= localrel)))
+=C2=A0 =C2=A0 =C2=A0 =C2=A0if (OidIsValid(rel->usableIndexOid))
=C2=A0 =C2=A0 =C2=A0 =C2=A0 {
```

Shouldn't we change the above comment to? The check is no longer slow.<= br>

Hmm, I couldn't realize this commen= t earlier. So you suggest "slow" here refers to the additional fu= nction call "GetRelationIdentityOrPK"? If so, yes I'll update= that.
=C2=A0

=3D=3D=3D
For relation.c

04. GetCheapestReplicaIdentityFullPath()

```
+static Path *
+GetCheapestReplicaIdentityFullPath(Relation localrel)
+{
+=C2=A0 =C2=A0 =C2=A0 =C2=A0PlannerInfo *root;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Query=C2=A0 =C2=A0 =C2=A0 *query;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0PlannerGlobal *glob;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0RangeTblEntry *rte;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0RelOptInfo *rel;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0int=C2=A0 =C2=A0 =C2=A0attno;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0RangeTblRef *rt;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0List *joinList;
+=C2=A0 =C2=A0 =C2=A0 =C2=A0Path *seqScanPath;
```

I think the part that constructs dummy-planner state can be move to another= function
because that part is not meaningful for this.
Especially line 824-846 can.


Makes sense, simplified the function. = Though, it is always hard to pick good names for these kinds of helper func= tions. I picked=C2=A0GenerateDummySelectPlannerInfoForRelation(), does that= sound good to you as well?=C2=A0
=C2=A0

=3D=3D=3D
For 032_subscribe_use_index.pl

05. general

```
+# insert some initial data within the range 0-1000
+$node_publisher->safe_psql('postgres',
+=C2=A0 =C2=A0 =C2=A0 =C2=A0"INSERT INTO test_replica_id_full SELECT i= %20 FROM generate_series(0,1000)i;"
+);
```

It seems that the range of initial data seems [0, 19].
Same mistake-candidates are found many place.

Ah, several copy & paste errors. Fixed (hopefully) all.
=C2=A0

06. general

```
+# updates 1000 rows
+$node_publisher->safe_psql('postgres',
+=C2=A0 =C2=A0 =C2=A0 =C2=A0"UPDATE test_replica_id_full SET x =3D x += 1 WHERE x =3D 15;");
```

Only 50 tuples are modified here.
Same mistake-candidates are found many place.

Alright, yes there were several wrong comments in the tests. I went = over the tests once more to fix those and improve comments.
=C2= =A0

07. general

```
+# we check if the index is used or not
+$node_subscriber->poll_query_until(
+=C2=A0 =C2=A0 =C2=A0 =C2=A0'postgres', q{select (idx_scan =3D 200)= from pg_stat_all_indexes where indexrelname =3D 'test_replica_id_full_= idx';}
+) or die "Timed out while waiting for check subscriber tap_sub_rep_fu= ll_3 updates 200 rows via index";
```
The query will be executed until the index scan is finished, but it may be = not commented.
How about changing it to "we wait until the index used on the subscrib= er-side." or something?
Same comments are found in many place.

= Makes sense, updated
=C2=A0

08. test related with ANALYZE

```
+# Testcase start: SUBSCRIPTION CAN UPDATE THE INDEX IT USES AFTER ANALYZE = - PARTITIONED TABLE
+# =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D= =3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D=3D
```

"Testcase start:" should be "Testcase end:" here.

thanks, fixed
=C2=A0

09. general

In some tests results are confirmed but in other test they are not.
I think you can make sure results are expected in any case if there are no = particular reasons.


Alright, yes I = also don't see a reason not to do that. Added to all cases.
<= br>

I'll attach the patch with the next email = as I also want to incorporate the other comments. Hope this is not going to= be confusing.

Thanks,
Onder
=
--0000000000001101f305e9096a43--