Re: Block level parallel vacuum

--000000000000d30331059c263bca
Content-Type: text/plain; charset="UTF-8"

On Tue, 14 Jan 2020 at 21:43, Amit Kapila <[email protected]> wrote:
>
> On Tue, Jan 14, 2020 at 10:04 AM Masahiko Sawada
> <[email protected]> wrote:
> >
> > On Mon, 13 Jan 2020 at 12:50, Amit Kapila <[email protected]> wrote:
> > >
> > > On Sat, Jan 11, 2020 at 7:48 PM Masahiko Sawada
> > > <[email protected]> wrote:
> > >
> > > Okay, would it better if we get rid of this variable and have code like below?
> > >
> > > /* Skip the indexes that can be processed by parallel workers */
> > > if ( !(get_indstats(lps->lvshared, i) == NULL ||
> > > skip_parallel_vacuum_index(Irel[i], lps->lvshared)))
> > >     continue;
> >
> > Make sense to me.
> >
>
> I have changed the comment and condition to make it a positive test so
> that it is more clear.
>
> > > ...
> > > > Agreed. But with the updated patch the PARALLEL option without the
> > > > parallel degree doesn't display warning because params->nworkers = 0
> > > > in that case. So how about restoring params->nworkers at the end of
> > > > vacuum_rel()?
> > > >
> > >
> > > I had also thought on those lines, but I was not entirely sure about
> > > this resetting of workers.  Today, again thinking about it, it seems
> > > the idea Mahendra is suggesting that is giving an error if the
> > > parallel degree is not specified seems reasonable to me.  This means
> > > Vacuum (parallel), Vacuum (parallel) <tbl_name>, etc. will give an
> > > error "parallel degree must be specified".  This idea has merit as now
> > > we are supporting a parallel vacuum by default, so a 'parallel' option
> > > without a parallel degree doesn't have any meaning.  If we do that,
> > > then we don't need to do anything additional about the handling of
> > > temp tables (other than what patch is already doing) as well.  What do
> > > you think?
> > >
> >
> > Good point! Agreed.
> >
>
> Thanks, changed accordingly.
>

Thank you for updating the patch! I have a few small comments. The
rest looks good to me.

1.
+ * Compute the number of parallel worker processes to request.  Both index
+ * vacuum and index cleanup can be executed with parallel workers.  The
+ * relation size of the table don't affect the parallel degree for now.

s/don't/doesn't/

2.
@@ -383,6 +435,7 @@ vacuum(List *relations, VacuumParams *params,
        VacuumPageHit = 0;
        VacuumPageMiss = 0;
        VacuumPageDirty = 0;
+       VacuumSharedCostBalance = NULL;

I think we can initialize VacuumCostBalanceLocal and
VacuumActiveNWorkers here. We use these parameters during parallel
index vacuum and reset at the end but we might want to initialize them
for safety.

3.
+   /* Set cost-based vacuum delay */
+   VacuumCostActive = (VacuumCostDelay > 0);
+   VacuumCostBalance = 0;
+   VacuumPageHit = 0;
+   VacuumPageMiss = 0;
+   VacuumPageDirty = 0;
+   VacuumSharedCostBalance = &(lvshared->cost_balance);
+   VacuumActiveNWorkers = &(lvshared->active_nworkers);

VacuumCostBalanceLocal also needs to be initialized.

4.
The regression tests don't have the test case of PARALLEL 0.

Since I guess you already modifies the code locally I've attached the
diff containing the above review comments.

Regards,

--
Masahiko Sawada            http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--000000000000d30331059c263bca
Content-Type: application/octet-stream; name="review_v47_masahiko.patch"
Content-Disposition: attachment; filename="review_v47_masahiko.patch"
Content-Transfer-Encoding: base64
Content-ID: <f_k5et58jy0>
X-Attachment-Id: f_k5et58jy0

ZGlmZiAtLWdpdCBhL3NyYy9iYWNrZW5kL2FjY2Vzcy9oZWFwL3ZhY3V1bWxhenkuYyBiL3NyYy9i
YWNrZW5kL2FjY2Vzcy9oZWFwL3ZhY3V1bWxhenkuYwppbmRleCBkMmM4OTVhNmZiLi5iZjU4NzNl
ZGIxIDEwMDY0NAotLS0gYS9zcmMvYmFja2VuZC9hY2Nlc3MvaGVhcC92YWN1dW1sYXp5LmMKKysr
IGIvc3JjL2JhY2tlbmQvYWNjZXNzL2hlYXAvdmFjdXVtbGF6eS5jCkBAIC0yOTM5LDcgKzI5Mzks
NyBAQCBoZWFwX3BhZ2VfaXNfYWxsX3Zpc2libGUoUmVsYXRpb24gcmVsLCBCdWZmZXIgYnVmLAog
LyoKICAqIENvbXB1dGUgdGhlIG51bWJlciBvZiBwYXJhbGxlbCB3b3JrZXIgcHJvY2Vzc2VzIHRv
IHJlcXVlc3QuICBCb3RoIGluZGV4CiAgKiB2YWN1dW0gYW5kIGluZGV4IGNsZWFudXAgY2FuIGJl
IGV4ZWN1dGVkIHdpdGggcGFyYWxsZWwgd29ya2Vycy4gIFRoZQotICogcmVsYXRpb24gc2l6ZSBv
ZiB0aGUgdGFibGUgZG9uJ3QgYWZmZWN0IHRoZSBwYXJhbGxlbCBkZWdyZWUgZm9yIG5vdy4KKyAq
IHJlbGF0aW9uIHNpemUgb2YgdGhlIHRhYmxlIGRvZXNuJ3QgYWZmZWN0IHRoZSBwYXJhbGxlbCBk
ZWdyZWUgZm9yIG5vdy4KICAqCiAgKiBucmVxdWVzdGVkIGlzIHRoZSBudW1iZXIgb2YgcGFyYWxs
ZWwgd29ya2VycyB0aGF0IHVzZXIgcmVxdWVzdGVkLiAgSWYKICAqIG5yZXF1ZXN0ZWQgaXMgMCwg
d2UgY29tcHV0ZSB0aGUgcGFyYWxsZWwgZGVncmVlIGJhc2VkIG9uIG5pbmRleGVzLCB0aGF0IGlz
CkBAIC0zMzY1LDYgKzMzNjUsNyBAQCBwYXJhbGxlbF92YWN1dW1fbWFpbihkc21fc2VnbWVudCAq
c2VnLCBzaG1fdG9jICp0b2MpCiAJVmFjdXVtUGFnZUhpdCA9IDA7CiAJVmFjdXVtUGFnZU1pc3Mg
PSAwOwogCVZhY3V1bVBhZ2VEaXJ0eSA9IDA7CisJVmFjdXVtQ29zdEJhbGFuY2VMb2NhbCA9IDA7
CiAJVmFjdXVtU2hhcmVkQ29zdEJhbGFuY2UgPSAmKGx2c2hhcmVkLT5jb3N0X2JhbGFuY2UpOwog
CVZhY3V1bUFjdGl2ZU5Xb3JrZXJzID0gJihsdnNoYXJlZC0+YWN0aXZlX253b3JrZXJzKTsKIApk
aWZmIC0tZ2l0IGEvc3JjL2JhY2tlbmQvY29tbWFuZHMvdmFjdXVtLmMgYi9zcmMvYmFja2VuZC9j
b21tYW5kcy92YWN1dW0uYwppbmRleCAxY2Q3N2U3OWQyLi5jNjQ1ZDQ0NjNmIDEwMDY0NAotLS0g
YS9zcmMvYmFja2VuZC9jb21tYW5kcy92YWN1dW0uYworKysgYi9zcmMvYmFja2VuZC9jb21tYW5k
cy92YWN1dW0uYwpAQCAtNDM1LDcgKzQzNSw5IEBAIHZhY3V1bShMaXN0ICpyZWxhdGlvbnMsIFZh
Y3V1bVBhcmFtcyAqcGFyYW1zLAogCQlWYWN1dW1QYWdlSGl0ID0gMDsKIAkJVmFjdXVtUGFnZU1p
c3MgPSAwOwogCQlWYWN1dW1QYWdlRGlydHkgPSAwOworCQlWYWN1dW1Db3N0QmFsYW5jZUxvY2Fs
ID0gMDsKIAkJVmFjdXVtU2hhcmVkQ29zdEJhbGFuY2UgPSBOVUxMOworCQlWYWN1dW1BY3RpdmVO
V29ya2VycyA9IE5VTEw7CiAKIAkJLyoKIAkJICogTG9vcCB0byBwcm9jZXNzIGVhY2ggc2VsZWN0
ZWQgcmVsYXRpb24uCmRpZmYgLS1naXQgYS9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL3ZhY3V1
bS5vdXQgYi9zcmMvdGVzdC9yZWdyZXNzL2V4cGVjdGVkL3ZhY3V1bS5vdXQKaW5kZXggMjJjY2E3
MDY4Ny4uNzc0ZmYyZmM3YyAxMDA2NDQKLS0tIGEvc3JjL3Rlc3QvcmVncmVzcy9leHBlY3RlZC92
YWN1dW0ub3V0CisrKyBiL3NyYy90ZXN0L3JlZ3Jlc3MvZXhwZWN0ZWQvdmFjdXVtLm91dApAQCAt
MTA3LDYgKzEwNyw4IEBAIFZBQ1VVTSAoUEFSQUxMRUwgMikgcHZhY3RzdDsKIC0tIFZBQ1VVTSBp
bnZva2VzIHBhcmFsbGVsIGJ1bGstZGVsZXRpb24KIFVQREFURSBwdmFjdHN0IFNFVCBpID0gaSBX
SEVSRSBpIDwgMTAwMDsKIFZBQ1VVTSAoUEFSQUxMRUwgMikgcHZhY3RzdDsKK1VQREFURSBwdmFj
dHN0IFNFVCBpID0gaSBXSEVSRSBpIDwgMTAwMDsKK1ZBQ1VVTSAoUEFSQUxMRUwgMCkgcHZhY3Rz
dDsgLS0gZGlzYWJsZSBwYXJhbGxlbCB2YWN1dW0KIFZBQ1VVTSAoUEFSQUxMRUwgLTEpIHB2YWN0
c3Q7IC0tIGVycm9yCiBFUlJPUjogIHBhcmFsbGVsIHZhY3V1bSBkZWdyZWUgbXVzdCBiZSBiZXR3
ZWVuIDAgYW5kIDEwMjQKIExJTkUgMTogVkFDVVVNIChQQVJBTExFTCAtMSkgcHZhY3RzdDsKZGlm
ZiAtLWdpdCBhL3NyYy90ZXN0L3JlZ3Jlc3Mvc3FsL3ZhY3V1bS5zcWwgYi9zcmMvdGVzdC9yZWdy
ZXNzL3NxbC92YWN1dW0uc3FsCmluZGV4IGQ2ODU5YTViYzkuLmZiZDM2NGQ3ZDAgMTAwNjQ0Ci0t
LSBhL3NyYy90ZXN0L3JlZ3Jlc3Mvc3FsL3ZhY3V1bS5zcWwKKysrIGIvc3JjL3Rlc3QvcmVncmVz
cy9zcWwvdmFjdXVtLnNxbApAQCAtOTMsNiArOTMsOSBAQCBWQUNVVU0gKFBBUkFMTEVMIDIpIHB2
YWN0c3Q7CiBVUERBVEUgcHZhY3RzdCBTRVQgaSA9IGkgV0hFUkUgaSA8IDEwMDA7CiBWQUNVVU0g
KFBBUkFMTEVMIDIpIHB2YWN0c3Q7CiAKK1VQREFURSBwdmFjdHN0IFNFVCBpID0gaSBXSEVSRSBp
IDwgMTAwMDsKK1ZBQ1VVTSAoUEFSQUxMRUwgMCkgcHZhY3RzdDsgLS0gZGlzYWJsZSBwYXJhbGxl
bCB2YWN1dW0KKwogVkFDVVVNIChQQVJBTExFTCAtMSkgcHZhY3RzdDsgLS0gZXJyb3IKIFZBQ1VV
TSAoUEFSQUxMRUwgMiwgSU5ERVhfQ0xFQU5VUCBGQUxTRSkgcHZhY3RzdDsKIFZBQ1VVTSAoUEFS
QUxMRUwgMiwgRlVMTCBUUlVFKSBwdmFjdHN0OyAtLSBlcnJvciwgY2Fubm90IHVzZSBib3RoIFBB
UkFMTEVMIGFuZCBGVUxMCg==
--000000000000d30331059c263bca--