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

Implement housekeeping interface for Caching fixes #4438 #4446

Merged
merged 3 commits into from Jan 24, 2018

Conversation

Projects
None yet
5 participants
@albertlast
Collaborator

albertlast commented Dec 30, 2017

and implement this for pg and also fix a bug in postgres cache table creation

@tinoest since you provide the sqlite cache maybe you could help me or add a own pr to add sqlite logic

Issue #4438

Implement housekeeping interface for Caching
and also fix a bug in postgres cache table creation
Signed-off-by: albertlast albertlast@hotmail.de

@Gwenwyfar Gwenwyfar added the Caching label Dec 30, 2017

@tinoest

This comment has been minimized.

Show comment
Hide comment
@tinoest

tinoest Dec 30, 2017

Contributor

I can later next week. Shouldn’t be to difficult. Why do you move to a new table?

Why not just delete old entries?

Contributor

tinoest commented Dec 30, 2017

I can later next week. Shouldn’t be to difficult. Why do you move to a new table?

Why not just delete old entries?

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Dec 30, 2017

Collaborator

I assume that the amount of data which are olded is "high".

Delete of data is slower and when you delete table of table,
you got data which is marked as deleted on the hdd without be "fully deleted".

See also: https://www.tutorialspoint.com/sqlite/sqlite_truncate_table.htm

Collaborator

albertlast commented Dec 30, 2017

I assume that the amount of data which are olded is "high".

Delete of data is slower and when you delete table of table,
you got data which is marked as deleted on the hdd without be "fully deleted".

See also: https://www.tutorialspoint.com/sqlite/sqlite_truncate_table.htm

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Dec 31, 2017

Member

Wouldn't it be better here to use a transaction for this? Since it could take a while to create the temp, empty the old table and recreate the new one? During that time any parallel calls to the cache table would cause some database errors possibly.

Member

jdarwood007 commented Dec 31, 2017

Wouldn't it be better here to use a transaction for this? Since it could take a while to create the temp, empty the old table and recreate the new one? During that time any parallel calls to the cache table would cause some database errors possibly.

Skip row when the data already exists
Signed-off-by: albertlast albertlast@hotmail.de
@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Dec 31, 2017

Collaborator

Let's go to the possible reason for transaction:
Is the data important? no (we can create the data when ever we like to do so)
What happen when someone select the data in the moment we truncate the table? Nothing he get the data and we truncate this table in the same moment.
Would it possible to do this in a transaction? In common no in the special case of postgresql yes.

The last point is very important for your to know.
SQL define that a transaction is only valid so long you do dml operation(delete, inset, update),
when you do ddl operation (create, alter, truncate...) the transaction get be commited.
But pg is here special and allow ddl inside a transaction.

So in theory could one case happen:
We truncate the table,
after this a user create a new cache entry and
then we insert the data from temp table and in the temp exists the samedata set(same key maybe different value).
In my eyes a very small chance that this happen,
but it's not hard catch this issue.
So i extend the pr about this.

Collaborator

albertlast commented Dec 31, 2017

Let's go to the possible reason for transaction:
Is the data important? no (we can create the data when ever we like to do so)
What happen when someone select the data in the moment we truncate the table? Nothing he get the data and we truncate this table in the same moment.
Would it possible to do this in a transaction? In common no in the special case of postgresql yes.

The last point is very important for your to know.
SQL define that a transaction is only valid so long you do dml operation(delete, inset, update),
when you do ddl operation (create, alter, truncate...) the transaction get be commited.
But pg is here special and allow ddl inside a transaction.

So in theory could one case happen:
We truncate the table,
after this a user create a new cache entry and
then we insert the data from temp table and in the temp exists the samedata set(same key maybe different value).
In my eyes a very small chance that this happen,
but it's not hard catch this issue.
So i extend the pr about this.

@jdarwood007

This comment has been minimized.

Show comment
Hide comment
@jdarwood007

jdarwood007 Dec 31, 2017

Member

Fair enough! Was just thinking about a race condition that could occur on very busy websites. Although I doubt they would be using pg caching. Most likely would be using another caching method then.

Member

jdarwood007 commented Dec 31, 2017

Fair enough! Was just thinking about a race condition that could occur on very busy websites. Although I doubt they would be using pg caching. Most likely would be using another caching method then.

@colinschoen colinschoen self-requested a review Jan 6, 2018

@albertlast

This comment has been minimized.

Show comment
Hide comment
@albertlast

albertlast Jan 24, 2018

Collaborator

@colinschoen @Sesquipedalian can we merge this?

Collaborator

albertlast commented Jan 24, 2018

@colinschoen @Sesquipedalian can we merge this?

@Sesquipedalian

This comment has been minimized.

Show comment
Hide comment
@Sesquipedalian

Sesquipedalian Jan 24, 2018

Member

@colinschoen assigned himself to review this, so I've been leaving it to him. :)

Member

Sesquipedalian commented Jan 24, 2018

@colinschoen assigned himself to review this, so I've been leaving it to him. :)

@Sesquipedalian Sesquipedalian merged commit 6952223 into SimpleMachines:release-2.1 Jan 24, 2018

2 checks passed

Scrutinizer 4 new issues, 7 updated code elements
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

Sesquipedalian added a commit that referenced this pull request Jan 24, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment