On Mon, Mar 16, 2020 at 01:53:35PM +0900, Masahiko Sawada wrote: > > In addition to comments from Michael-san, here are my comments: > > 1. > + if (!is_member_of_role(GetUserId(), DEFAULT_ROLE_READ_SERVER_FILES)) > + ereport(ERROR, > + (errcode(ERRCODE_INSUFFICIENT_PRIVILEGE), > + errmsg("only superuser or a member of the > pg_read_server_files role may use this function")));
Good point! I'll fix it. > + > + if (!DataChecksumsEnabled()) > + elog(ERROR, "Data checksums are not enabled"); > > I think it's better to reverse the order of the above checks. Indeed. > > 2. > +#define CRF_COLS 5 /* Number of output arguments in the SRF */ > > Should it be SRF_COLS? Oops, will fix. > > 3. > +static void > +check_delay_point(void) > +{ > + /* Always check for interrupts */ > + CHECK_FOR_INTERRUPTS(); > + > + /* Nap if appropriate */ > + if (!InterruptPending && VacuumCostBalance >= VacuumCostLimit) > + { > + int msec; > + > + msec = VacuumCostDelay * VacuumCostBalance / VacuumCostLimit; > + if (msec > VacuumCostDelay * 4) > + msec = VacuumCostDelay * 4; > + > + pg_usleep(msec * 1000L); > + > + VacuumCostBalance = 0; > + > + /* Might have gotten an interrupt while sleeping */ > + CHECK_FOR_INTERRUPTS(); > + } > +} > > Even if we use vacuum delay for this function, I think we need to set > VacuumDelayActive and return if it's false, or it's better to just > return if VacuumCostDelay == 0. Good point, I'll fix that. > > 4. > +static void > +check_all_relations(TupleDesc tupdesc, Tuplestorestate *tupstore, > + ForkNumber forknum) > > I also agree with Michael-san to remove this function. Instead we can > check all relations by: > > select pg_check_relation(oid) from pg_class; Sure, but ideally we should do that in a client program (eg. pg_checksums) that wouldn't maintain a transaction active for the whole execution. > 6. > Other typos > > s/dirted/dirtied/ > s/explictly/explicitly/ Will fix, thanks!