Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: allow disabling field metadata cache #1052

Merged
merged 5 commits into from
May 9, 2018

Conversation

znep
Copy link
Contributor

@znep znep commented Jan 3, 2018

Clients accessing very dynamic schemas can have issues with the
field metadata cache getting stale. This change allows configuring the
databaseMetadataCacheFields property to 0 to disable the cache and
avoid these issues where necessary. This behaviour was already
documented, however did not actually work as the codepath assumed
it could retrieve the fields from the cache.

@davecramer
Copy link
Member

How do we test this ?

@znep
Copy link
Contributor Author

znep commented Jan 3, 2018

@davecramer Good question, I was unsure the extent of testing that is typical for modifying previously untested code paths in this project so was going to add a follow-up comment asking for input.

I can certainly make a test that creates a table, reads from it, modifies the data type of a column, then reads from it again and ensures that the correct new type is returned when caching is disabled and the old "incorrect" value is returned when caching is enabled.

Does that seem sufficient? It wouldn't be full coverage of the caching codepath but we would have a test that fails before the fix and passes after, and one that "passes" both before and after the fix.

@davecramer
Copy link
Member

@znep we'd certainly like to make sure that the codepath has some testing and that this couldn't occur again due to some change in the future.

Your proposed test sounds like a great start!

@vlsi
Copy link
Member

vlsi commented Jan 8, 2018

As far as I read the code, a mere databaseMetadataCacheFields=0 + regular metadata test should be enough.

That is it makes sense to add @Parameterized to some of the existing tests and parameterize over databaseMetadataCacheFields values like we do for binary, etc, etc.

@znep , any luck with pushing this forward?

@davecramer
Copy link
Member

@znep ping ?

@znep
Copy link
Contributor Author

znep commented Jan 14, 2018

Hi guys,

I haven't had a chance to circle back to this yet but still plan to this week, I'll take a look at @vlsi 's suggestion.

Clients accessing very dynamic schemas can have issues with the
field metadata cache getting stale.  This change allows configuring the
databaseMetadataCacheFields property to 0 to disable the cache and
avoid these issues where necessary.  This behaviour was already
documented, however did not actually work as the codepath assumed
it could retrieve the fields from the cache.
@codecov-io
Copy link

Codecov Report

Merging #1052 into master will increase coverage by 0.04%.
The diff coverage is 100%.

@@             Coverage Diff              @@
##             master    #1052      +/-   ##
============================================
+ Coverage     69.14%   69.18%   +0.04%     
- Complexity     3790     3797       +7     
============================================
  Files           170      171       +1     
  Lines         15760    15765       +5     
  Branches       2578     2578              
============================================
+ Hits          10897    10907      +10     
+ Misses         3621     3618       -3     
+ Partials       1242     1240       -2

@znep
Copy link
Contributor Author

znep commented Apr 15, 2018

@davecramer @vlsi None of the existing ResultSetMetaDataTest tests seem to have quite the right cases to repro this, so I added a new test case and parameterized the test suite.

I verified the tests fail as I expect without my fix.

@znep
Copy link
Contributor Author

znep commented May 8, 2018

@davecramer @vlsi ping, anything else I should add here? Thanks.

@vlsi
Copy link
Member

vlsi commented May 9, 2018

@znep , it looks good, however I wonder if fieldInfoFetched should still be set to true after metadata fetch from the database.

I think the end of populateFieldsWithMetadata should read fieldInfoFetched |= allOk;

What do you think?

This is a minor optimization, previously the second call to
fetchFieldMetaData would iterate through each field and see it was
already set, set fieldInfoFetched to true and return.
@znep
Copy link
Contributor Author

znep commented May 9, 2018

@vlsi Good catch, thanks for looking. I agree it should be, this isn't a regression from my changes and is a pretty minor optimization (it doesn't actually refetch it from the cache or db, just iterates through all the fields and sees they are already set) but I pushed a fix.

I'm not seeing a way to add test coverage for this without some fairly major structural changes.

(build currently failing due to travis apt-get issues...)

@vlsi
Copy link
Member

vlsi commented May 9, 2018

As a follow-up it might make sense to add metadata cache invalidation on ALTER statements.

@vlsi vlsi merged commit 6ce9172 into pgjdbc:master May 9, 2018
@vlsi vlsi added this to the 42.2.3 milestone May 9, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants