I noticed that when configured batch_size is reduced because of libpq limit, in postgresGetForeignModifyBatchSize, there is no message about it that gets to the user. I am thinking that to keep the transparency it makes sense to add a debug message at least, as per the attached patch.
What do you think?
I'm iffy on this one.
The desire for instrumentation is generally good, and if a database was having a problem where the tuned parameter wasn't having the desired effect then it's understandable to want to know that.
But I'm also not certain what different action someone would take if they knew that this situation was happening. Do you anticipate some sort of action on the user or DBA's part from this information beyond understanding that's why you're not getting the full batch size?
Hey Corey,
Thanks for your opinion. As I said, it feels important from the transparency point to know when and why the batch_size changed for a case.
Thoughts on the code change itself:
- The new variable (configured) should be declared in the new if/then block, which is the narrowest scope possible.
- The ereport() uses the old style with errmsg inside parens, and that's no longer necessary. Existing ereport calls were left unchanged to avoid code churn, but new ones should be in the simpler style.
- This error is debuggy-enough that it might be an elog() rather than an ereport()
- I don't have good guidance on whether it should be DEBUG1 vs DEBUG2 or a higher number. Curious if we have that guidance documented anywhere.
- In the test case, choosing a batch_size > PQ_QUERY_PARAM_MAX_LIMIT seems a bit artificial, in that it's a value that can't work for any query with parameters, and someone might add sanity checks to batch_size in option.c, which would in turn invalidate the test case. A test case with a 2-column table should be able to get the same effect any batch_size greater than half of 65535, yes?
