On Mon, Nov 26, 2018 at 3:46 PM John Naylor <jcnay...@gmail.com> wrote: > > On 11/24/18, Amit Kapila <amit.kapil...@gmail.com> wrote: > > On Fri, Nov 23, 2018 at 11:56 AM John Naylor <jcnay...@gmail.com> wrote: > >> On 11/16/18, Amit Kapila <amit.kapil...@gmail.com> wrote: > >> > >> > > >> > 3. > >> > static int fsm_set_and_search(Relation rel, FSMAddress addr, uint16 > >> > slot, > >> > - uint8 newValue, uint8 minValue); > >> > + uint8 newValue, uint8 minValue); > >> > > >> > This appears to be a spurious change. > >> > >> 2 and 3 are separated into 0001. > >> > > > > Is the point 3 change related to pgindent? I think even if you want > > these, then don't prepare other patches on top of this, keep it > > entirely separate. > > There are some places in the codebase that have spaces after tabs for > no apparent reason (not to line up function parameters or pointer > declarations). pgindent hasn't been able to fix this. If you wish, you > are of course free to apply 0001 separately at any time. >
I am not sure that I am interested in generally changing the parts of code that are not directly related to this patch, feel free to post them separately. > >> Also, we don't quite have a consensus on the threshold value, but I > >> have set it to 4 pages for v8. If this is still considered too > >> expensive (and basic tests show it shouldn't be), I suspect it'd be > >> better to interleave the available block numbers as described a couple > >> days ago than lower the threshold further. > >> > > > > Can you please repeat the copy test you have done above with > > fillfactor as 20 and 30? > > I did two kinds of tests. The first had a fill-factor of 10 [1], the > second had the default storage, but I prevented the backend from > caching the target block [2], to fully exercise the free space code. > Would you like me to repeat the first one with 20 and 30? > Yes. > And do you > think it is useful enough to test the copying of 4 blocks and not > smaller numbers? > You can try that, but I am mainly interested with 4 as threshold or may be higher to see where we start loosing. I think 4 should be a reasonable default for this patch, if later anyone wants to extend, we might want to provide a table level knob. > > Few more comments: > > ------------------------------- > > 1. I think we can add some test(s) to test the new functionality, may > > be something on the lines of what Robert has originally provided as an > > example of this behavior [1]. > > Maybe the SQL script attached to [3] (which I probably based on > Robert's report) can be cleaned up into a regression test. > Yeah, something on those lines works for me. > > 3. > > GetPageWithFreeSpace(Relation rel, Size spaceNeeded) > > { > > .. > > + if (target_block == InvalidBlockNumber && > > + rel->rd_rel->relkind == RELKIND_RELATION) > > + { > > + nblocks = RelationGetNumberOfBlocks(rel); > > + > > + if (nblocks > HEAP_FSM_CREATION_THRESHOLD) > > + { > > + /* > > + * If the FSM knows nothing of the rel, try the last page before > > + * we give up and extend. This avoids one-tuple-per-page syndrome > > + * during bootstrapping or in a recently-started system. > > + */ > > + target_block = nblocks - 1; > > + } > > .. > > } > > > > Moving this check inside GetPageWithFreeSpace has one disadvantage, we > > will always consider last block which can have some inadvertent > > effects. Consider when this function gets called from > > RelationGetBufferForTuple just before extension, it can consider to > > check for the last block even though that is already being done in the > > begining when GetPageWithFreeSpace was called. I am not completely > > sure how much this is a case to worry because it will help to check > > last block when the same is concurrently added and FSM is not updated > > for same. I am slightly worried because the unpatched code doesn't > > care for such case and we have no intention to change this behaviour. > > What do you think? > > I see what you mean. If the other backend extended by 1 block, the > intention is to keep it out of the FSM at first, and by extension, not > visible in other ways. The comment implies that's debatable, but I > agree we shouldn't change that without a reason to. One simple idea is > add a 3rd boolean parameter to GetPageWithFreeSpace() to control > whether it gives up if the FSM fork doesn't indicate free space, like > > if (target_block == InvalidBlockNumber && > rel->rd_rel->relkind == RELKIND_RELATION && > !check_fsm_only) > { > nblocks = RelationGetNumberOfBlocks(rel); > I have the exact fix in my mind, so let's do it that way. > > 4. You have mentioned above that "system catalogs created during > > bootstrap still have a FSM if they have any data." and I can also see > > this behavior, have you investigated this point further? > > Code reading didn't uncover the cause. I might have to step through > with a debugger or something similar. I should find time for that next > month. > Okay. > > 5. Your logic to update FSM on standby seems okay, but can you show > > some tests which proves its sanity? > > I believe to convince myself it was working, I used the individual > commands in the sql file in [3], then used the size function on the > secondary. I'll redo that to verify. > Okay. -- With Regards, Amit Kapila. EnterpriseDB: http://www.enterprisedb.com